fix: update the Transaction RPC type and fix response of txpool_content RPC#1234
Merged
sorpaas merged 2 commits intopolkadot-evm:masterfrom Nov 2, 2023
koushiro:fix-transaction-type-and-txpool-content
Merged
fix: update the Transaction RPC type and fix response of txpool_content RPC#1234sorpaas merged 2 commits intopolkadot-evm:masterfrom koushiro:fix-transaction-type-and-txpool-content
sorpaas merged 2 commits intopolkadot-evm:masterfrom
koushiro:fix-transaction-type-and-txpool-content
Conversation
koushiro
commented
Oct 27, 2023
Comment on lines
-75
to
-76
| #[cfg_attr(feature = "std", serde(skip_serializing_if = "Option::is_none"))] | ||
| pub access_list: Option<Vec<AccessListItem>>, |
Collaborator
Author
There was a problem hiding this comment.
I don't know why we used #[cfg_attr(feature = "std", ...) here before, maybe it was an error caused by copy c + v
koushiro
commented
Oct 27, 2023
| match transaction { | ||
| TransactionV2::Legacy(t) => Transaction { | ||
| EthereumTransaction::Legacy(t) => Transaction { | ||
| transaction_type: Some(U256::from(0)), |
Collaborator
Author
There was a problem hiding this comment.
I'm not sure if we should use transaction_type : None for Legacy transaction
Member
There was a problem hiding this comment.
It should be 0x0.
From what I see in the RPC spec, this field is never None, so we can probably remove the Option.
Collaborator
Author
- fix the response of txpool_content RPC - update the `Transaction` response type of some RPC response
Collaborator
Author
|
@sorpaas PTAL |
sorpaas
reviewed
Nov 1, 2023
Member
sorpaas
left a comment
sorpaas
left a comment
There was a problem hiding this comment.
LGTM. Grumble on transaction_type.
Collaborator
Author
|
@sorpaas PTAL again |
koushiro
added a commit
to alt-research/frontier
that referenced
this pull request
Nov 2, 2023
…nt RPC (polkadot-evm#1234) * fix: update the Transaction type and fix response of txpool_content RPC - fix the response of txpool_content RPC - update the `Transaction` response type of some RPC response * remove Option wrapper of transaction type
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
When using
txpool_contentRPC, I found that its return value is not compatible withgeth/nethermindclient,so I try to fix it.
But when I read the implementation code of geth, I found that the transaction RPC type implemented by
frontieris also different from other clients, and even different from the description in the ethereum api documentation.Like
public_key: it's not included in the ethereum api doc, and it could be recovered from signature and raw txnraw: it's is not included in the ethereum api doc and I think it's redundantstandard_v: it should be namedy_parityorv(for compatibility) according to the ethereum api doccreates: it's not included in the ethereum api doc, but I keep itWhat I did
TransactionRPC type