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

Fix eth_signTransaction request and response #7804

Merged
merged 4 commits into from
May 1, 2024

Conversation

alisinabh
Copy link
Contributor

@alisinabh alisinabh commented Apr 28, 2024

Motivation

As per Ethereum JSON-RPC API Specification, the eth_signTransaction method should return an RLP encoded transaction payload. Anvil is just returning the signature.

Also the parameters of the transaction MUST be part of a sequence.

Solution

This PR returns the signed payload in eth_signTransaction request and also configures the deserializer to deserialize params using a sequence.

A happy path test was also added in tests/it/sign.rs (Was not sure if I should put the test there or in tests/it/transaction.rs)

Thank you!

@alisinabh alisinabh changed the title Fix eth_signTransaction request and response Fix eth_signTransaction request and response Apr 28, 2024
@alisinabh
Copy link
Contributor Author

I think there still is a bug (probably in alloy) for signing EIP1559 transactions. The signed result (like the example added here in test) has an invalid y_parity value. It looks like alloy is returning an EIP155 enum value when signing a EIP1559 transaction. For EIP1559, the parity value should be a true/false since chain_id value is already present in the payload.

@alisinabh alisinabh requested a review from DaniPopes April 29, 2024 00:54
@alisinabh
Copy link
Contributor Author

I think there still is a bug (probably in alloy) for signing EIP1559 transactions. The signed result (like the example added here in test) has an invalid y_parity value. It looks like alloy is returning an EIP155 enum value when signing a EIP1559 transaction. For EIP1559, the parity value should be a true/false since chain_id value is already present in the payload.

The change in this PR (alloy-rs/alloy#647) fixes the issue above with signing. I'm not sure if it was the correct place to do that and opened that PR for discussion.

@alisinabh
Copy link
Contributor Author

Sorry to ping again @DaniPopes. Please let me know if anything else is required and if I can help further with the EIP155 issue in transaction signatures.

Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

thanks, @mattsse ptal

@mattsse mattsse merged commit 5885dbc into foundry-rs:master May 1, 2024
19 checks passed
0xgregthedev pushed a commit to phylaxsystems/phoundry that referenced this pull request May 2, 2024
* Fix eth_signTransaction request and response

* fixup! Fix eth_signTransaction request and response

* Hardcode test nonce and fee values

* Fix test signed result
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.

3 participants