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

[WIP]Feat: add EIP-1559 transaction type #12

Conversation

haythemsellami
Copy link
Contributor

No description provided.

Copy link
Collaborator

@ferranbt ferranbt left a comment

Choose a reason for hiding this comment

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

Hey! Thank you for your contribution! That was fast!

Congratulations on being the first external contributor.

I left a couple of comments. Could you also include a test to validate the encoding/decoding?

@@ -38,7 +53,26 @@ library Transactions {
return RLPWriter.writeList(items);
}

function decodeRLP(bytes memory rlp) internal pure returns (Legacy memory) {
function encodeRLP(EIP1559 memory txStruct) internal pure returns (bytes memory) {
bytes[] memory items = new bytes[](12);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this encoding correct? EIP-1559 transactions are encoded with EIP-2718 format, so it needs to have the transaction type prefixed. You can find a code reference here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just fixed this, lmk if it is still not correct.

bytes v;
}

function encodeLegacyRLP(Legacy memory txStruct) internal pure returns (bytes memory) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you keep the same name here? I imagine having the same encodeRLP name overloaded for both transactions.

@ferranbt
Copy link
Collaborator

ferranbt commented Jan 5, 2024

I made some changes to the encoding of legacy transactions. You'll need to pull the new changes.

@haythemsellami
Copy link
Contributor Author

I made some changes to the encoding of legacy transactions. You'll need to pull the new changes.

Cool will do.

@ferranbt
Copy link
Collaborator

Hey @haythemsellami. Do you need any help on the PR?

@haythemsellami
Copy link
Contributor Author

Hey @haythemsellami. Do you need any help on the PR?

Trying to finish the test and make it working, but I may have a bug in the encoding implementation as the output is not matching(maybe the access list encoding is wrong?)

@haythemsellami
Copy link
Contributor Author

Closing this in favour of #36.

@haythemsellami haythemsellami deleted the feat/eip1559-transaction branch January 25, 2024 06:16
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.

2 participants