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: 1559/2930 txs (part 2) #355

Merged
merged 10 commits into from Aug 8, 2021
Merged

feat: 1559/2930 txs (part 2) #355

merged 10 commits into from Aug 8, 2021

Conversation

gakonst
Copy link
Owner

@gakonst gakonst commented Jul 31, 2021

  • Introduces the eip1559/2930 tx types
  • Introduces a TypedTransaction enum w/ From implementations for each type, which will replace TransactionRequest in the rest of the codebase
  • Fixes a bug pointed out by @guanqun on the AccessList RLP encoding derivation method
  • Adds tests to ensure that RLP encoding and the sighash matches Geth's

@gakonst gakonst changed the title feat: 1559/2930 txs feat: 1559/2930 txs (part 2) Aug 1, 2021
Legacy(TransactionRequest),
// 0x01
#[serde(rename = "0x01")]
Eip2930(Eip2930TransactionRequest),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can I request a rename of these transaction names?

I prefer python's version of "AccessListTransactionRequest" and "DynamicFeeTransactionRequest" over these number named one.

Eip2930 and Eip1559 are not straightforward, it's hard to figure out the underlying meaning.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

Hey - we discussed it in the dev chat, I preferred the EIPxxx numbering, confirmed by @prestwich @roynalnaruto @mattsse, so we'll keep the EIPXXX numbering for tx types!

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.

only checked syntax, not the actual encoding.

use serde::{Deserialize, Serialize};
/// Parameters for sending a transaction
#[derive(Clone, Default, Serialize, Deserialize, PartialEq, Eq, Debug)]
pub struct Eip1559TransactionRequest {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
pub struct Eip1559TransactionRequest {
#[serde(rename_all = "camelCase")]
pub struct Eip1559TransactionRequest {

instead of individual renames?

@guanqun
Copy link
Contributor

guanqun commented Aug 3, 2021

Thank you for the feature.

My only comment is the naming. These EIP numbers are just magic numbers, we should avoid using magic numbers.

How do you think? @gakonst @mattsse

If you think it's tedious, I can help rename them at once.

@gakonst
Copy link
Owner Author

gakonst commented Aug 3, 2021

@guanqun replied above, let's stick with the EIP numbers, they're numbers that everyone in Ethereum knows about (EIP-1559 is very specific about what it gives, vs "Dynamic Fee" can mean literally anything)

@guanqun
Copy link
Contributor

guanqun commented Aug 3, 2021

Thanks for the info.

I agree that EIP-1559 is popular, but from my own view, other two EIP-2930 and EIP-2718 are less popular. I doubt that people would immediately know the meaning behind them.

That said, you have the final call.

@guanqun
Copy link
Contributor

guanqun commented Aug 4, 2021

FYI. A few small changes are needed to make it work on goerli network. The biggest one missing here is the leading header byte. I hack around and come up with a patch here, if it's useful to others. guanqun@68de2a2

Base automatically changed from feat/typed-txs-types to master August 8, 2021 22:49
@gakonst gakonst merged commit ccff9fc into master Aug 8, 2021
@gakonst gakonst deleted the feat/typed-txs-enum branch August 8, 2021 22:59
meetmangukiya pushed a commit to meetmangukiya/ethers-rs that referenced this pull request Mar 21, 2022
* refactor: move format_token to utils

* refactor(cli): use foundry_utils::format_tokens

* fix(forge): pretty print counterexample

* fix(forge): print string with quotation marks and add brackets

* fix(forge): do not set the revert reason if it's empty

* chore: cargo fmt / lint
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