Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Required Fields to SignedTransaction Type #9

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

bh2smith
Copy link
Contributor

@bh2smith bh2smith commented Nov 14, 2023

As described in #8, we find the transaction hash being returned by getBlockByNumber.

sample request
curl -X POST $NODE_URL --data '{"jsonrpc":"2.0","method":"eth_getBlockByNumber","params":["0x119f996", true],"id":1}' | jq

sample response: http://jsonblob.com/1173956359015489536

This PR adds the hash and from fields to all SignedTransaction types.

Closes #8

Also reported this inconsistency to the base docs: ethereum/execution-apis#492

I also lifted these common fields out into public methods (so we don't have to match all the time when trying to access them).

Note

It is becoming more clear that SignedTransaction is not the type we should be updating here. It almost seems more like the function returns TransactionReceipts or Transaction type. We are finding fields like transactionIndex being returned which are certainly not part of a SignedTransaction. We can see that the type returned by this call is Transaction in the ethers crate

Further Issues

Now, it seems that this change does not work with QuickNode or Infura while it works seamlessly with Ankr. The reason appears to be that both QuickNode and Infura return BOTH "v" and "yParity" fields (see transaction 0 of

curl -X POST $INFURA --data '{"jsonrpc":"2.0","method":"eth_getBlockByNumber","params":["0xe4e364", true],"id":1}' 

curl -X POST $ANKR --data '{"jsonrpc":"2.0","method":"eth_getBlockByNumber","params":["0xe4e364", true],"id":1}'

with | jq '.result.transactions[0]'

or see diff

Blahhhh...

the type is declared with

    /// Y parity of the signature.
    #[serde(alias = "v")]
    pub y_parity: YParity,

Which seems to cause conflict when both fields are present. Is there some way to intercept the responses and eliminate one of the two if they are both there? This is becoming such a mess.

I am going to report this as a separate independent bug since it affects the current main branch already without this change.

@bh2smith bh2smith changed the title Add Hash field to SignedTransaction Type Add Required Fields to SignedTransaction Type Nov 14, 2023
@@ -245,6 +245,71 @@ pub enum SignedTransaction {
Eip4844(SignedEip4844Transaction),
}

impl SignedTransaction {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this feels like a code smell that indicates the types should be refactored somehow. Let me think about it a bit, I may have some nice alternatives to this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hydrated Blocks Contain More than Parsed
2 participants