Skip to content

feat(tx): add EIP-1559 transaction with priority fee support#1

Merged
shamardy merged 27 commits into
GLEECBTC:masterfrom
dimxy:eip1559-support
Apr 5, 2024
Merged

feat(tx): add EIP-1559 transaction with priority fee support#1
shamardy merged 27 commits into
GLEECBTC:masterfrom
dimxy:eip1559-support

Conversation

@dimxy
Copy link
Copy Markdown
Collaborator

@dimxy dimxy commented Mar 23, 2024

Add typed transactions support for type 1 with EIP-2930 access lists and type 2 of EIP-1559 fee per gas changes

@laruh
Copy link
Copy Markdown

laruh commented Mar 26, 2024

@dimxy please tell which rust toolchain version did you use?
As I can see in the project is used old config version rust-toolchain with just one word stable, and not rust-toolchain.toml.
Well it uses default stable version (I have this rustc 1.76.0 (07dca489a 2024-02-04) stable) but still.
I suggest to update it to rust-toolchain.toml and specify version, and in this way to commit cargo lock file.
Also its better to add edition version in cargo tomls, as without it cargo uses 2015 version by default.

As I understood you cant expect to format code in the whole project with cargo fmt command, as root Cargo.toml doesnt have [workspace] members. lib.rs is just empty file
image
Well we dont have to create workspace members.
You can format ethkey and ethcore folders running these scripts, as we still can use fmt command by itself.

find ./ethcore/transaction/src -type f -name '*.rs' -exec rustfmt {} +
find ./ethkey/src -type f -name '*.rs' -exec rustfmt {} +

But if we decide to have workspaces, then we will need to additionally write package in komodefi deps.
(ps Its just the info, i dont think we need it.)

[dependencies]
ethkey = { git = "https://github.com/dimxy/mm2-parity-ethereum.git", branch = "eip1559-support", package = "ethkey" }
ethcore-transaction = { git = "https://github.com/dimxy/mm2-parity-ethereum.git", branch = "eip1559-support", package = "ethcore-transaction" }

PS: yeah, I understand that rust-toolchain in dependency project doesn't affect the dependent project, and dependencies are compiled with dependent project own toolchain. Its is primarily for developers working with the project directly.

I have warning related to https://github.com/paritytech/ringthat it was not used in the crate graph. Do you have the same?
image

@dimxy
Copy link
Copy Markdown
Collaborator Author

dimxy commented Mar 26, 2024

@dimxy please tell which rust toolchain version did you use?

stable-x86_64-apple-darwin (overridden by '/Users/dimxy/repo/mm2-parity-ethereum/rust-toolchain')

@laruh
Copy link
Copy Markdown

laruh commented Mar 28, 2024

UPD: sorry, no need in rust-toolchain.toml as we are going to make this repo part of komodefi

@dimxy
Copy link
Copy Markdown
Collaborator Author

dimxy commented Mar 28, 2024

UPD: sorry, no need in rust-toolchain.toml as we are going to make this repo part of komodefi

maybe, until we do not move it to our repo, it is worth to fix the toolchain (as this repo has not had any updates for a long time)

@laruh
Copy link
Copy Markdown

laruh commented Mar 28, 2024

UPD: sorry, no need in rust-toolchain.toml as we are going to make this repo part of komodefi

maybe, until we do not move it to our repo, it is worth to fix the toolchain (as this repo has not had any updates for a long time)

Well if its ok for you. It didnt take much time for me to fix clippy warnings for 1.76.0 stable version locally.
then you should add toolchain.toml with specific version in channel, also not bad to add components = ["rustfmt", "clippy"] and add edition versions in Cargo tomls.

As for [workspace] members in root Caro.toml its up to you. you can use scripts which I provided to format code in necessary folders.

@dimxy
Copy link
Copy Markdown
Collaborator Author

dimxy commented Mar 28, 2024

If I set it to our "nightly-2022-10-29" I begin to receive that 'patch ring was not used in the crate graph' warning. I guess I did not receive it myself because I had some older default toolchain picked by this lib 'stable' setting. I think this is not good just to use any 'stable' ver and better to fix it to some specific rust ver where this warning does not appear (it would be probably older than ours).

@laruh
Copy link
Copy Markdown

laruh commented Mar 28, 2024

If I set it to our "nightly-2022-10-29" I begin to receive that 'patch ring was not used in the crate graph' warning. I guess I did not receive it myself because I had some older default toolchain picked by this lib 'stable' setting. I think this is not good just to use any 'stable' ver and better to fix it to some specific rust ver where this warning does not appear (it would be probably older than ours).

I see it with 1.76.0 stable version. And sometimes after cargo clean didnt see it, then saw it again. I think it is just there and independent to toolchain version

@dimxy
Copy link
Copy Markdown
Collaborator Author

dimxy commented Mar 29, 2024

As I can tell cargo clippy complains about the used version of the 'ring' lib like it is different from the version in cargo.lock or just outdated. In fact this lib is fetched from the parity repo (also cargo.lock looks correct as it points to this repo too). I guess it's not good to just change it to a newer version so I suggest ignoring this warning for now.
Btw we have plans to switch to a different eth lib in future like alloy-rs whene it is ready.

@laruh
Copy link
Copy Markdown

laruh commented Mar 29, 2024

I guess it's not good to just change it to a newer version so I suggest ignoring this warning for now.
Btw we have plans to switch to a different eth lib in future like alloy-rs whene it is ready.

Yep, agree.

PS: commit the Cargo.lock please

Comment thread ethcore/transaction/src/transaction/eip1559.rs Outdated
Comment thread ethcore/transaction/src/transaction/eip1559.rs Outdated
shamardy
shamardy previously approved these changes Apr 4, 2024
Copy link
Copy Markdown
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

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

LGTM apart from a few nits!

Comment thread ethkey/src/error.rs Outdated
Comment thread ethcore/transaction/src/transaction.rs
Comment thread ethcore/transaction/src/transaction/eip1559.rs Outdated
Comment thread ethcore/transaction/src/transaction/eip2930.rs Outdated
laruh
laruh previously approved these changes Apr 5, 2024
Copy link
Copy Markdown

@laruh laruh left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes, LGTM!

non blocking suggestion, please add some info in PR description. that additionally to eip-1559 tx the eip-2930 also was added. now project contains 3 versions of tx.

added doc comments to tx types
@dimxy dimxy dismissed stale reviews from laruh and shamardy via 5dbe940 April 5, 2024 07:08
@dimxy
Copy link
Copy Markdown
Collaborator Author

dimxy commented Apr 5, 2024

non blocking suggestion, please add some info in PR description. that additionally to eip-1559 tx the eip-2930 also was added. now project contains 3 versions of tx.

added

@shamardy shamardy merged commit 931d2f6 into GLEECBTC:master Apr 5, 2024
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