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

feat(core): implemented signed transaction RLP decoding #1096

Merged
merged 3 commits into from Apr 9, 2022

Conversation

Rjected
Copy link
Contributor

@Rjected Rjected commented Mar 30, 2022

Motivation

There are often cases where we might have some RLP encoding of a transaction, for example when receiving P2P messages, and want to decode that into a transaction type.

Solution

This implements RLP decoding for signed transactions, extracting the signature from the decoding as well as building and returning the transaction. This also recovers the sender from the signature so we can populate the from field. This can error, so we introduce new error types (Eip1559RequestError, TypedTransactionError, RequestError) which also wrap errors we might encounter when decoding and recovering the sender.

This also introduces a hash method on TypedTransaction which returns the hash of the signed transaction's RLP encoding.

PR Checklist

  • Added Tests
  • Added Documentation
  • Updated the changelog

@Rjected Rjected force-pushed the signed_transaction_decoding branch from 678aaff to 1b6dddb Compare March 30, 2022 02:00
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

awesome!

before we merge this, mind having a look at this

https://github.com/gakonst/foundry/blob/71f9e89256815008b5e4e7f8de84513c13110793/node/node-core/src/eth/transaction.rs

that's part of the forge node PR foundry-rs/foundry#1037

whether we can somehow unify some things

@gakonst
Copy link
Owner

gakonst commented Apr 2, 2022

@Rjected have you had the chance to look at Matt's code above? Are there maybe more utils to add?

@gakonst
Copy link
Owner

gakonst commented Apr 5, 2022

(bump @Rjected)

@mattsse
Copy link
Collaborator

mattsse commented Apr 5, 2022

thanks @Rjected for the write up here foundry-rs/foundry#1037 (comment)

agree with most of that, current blocker is that I need to convert from foundry node TypedTransaction(Request) to ethers Transaction(Request), in order to reuse ether's signers.

I think for now I'll try to do conversion in both ways and figure out how to unify afterwards, because I believe it could be replaced with ethers' types eventually.

so my recommendation, get this PR and the node PR over the line individually and unify and clean up afterwards.

Copy link
Owner

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

lgtm if @mattsse thinks this is OK to merge as-is - need to fix minor conflict in changelog

@Rjected Rjected force-pushed the signed_transaction_decoding branch from 1b6dddb to eeb8d37 Compare April 9, 2022 00:55
@Rjected Rjected force-pushed the signed_transaction_decoding branch from eeb8d37 to f50da3b Compare April 9, 2022 01:00
@Rjected
Copy link
Contributor Author

Rjected commented Apr 9, 2022

Cool, just fixed changelog, rebased

Copy link
Owner

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

based rebase, lgtm thank you for getting this over the line

@gakonst gakonst merged commit 6e004e7 into gakonst:master Apr 9, 2022
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.

None yet

3 participants