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

EIP1559 New fee support #1509

Merged
merged 37 commits into from
Aug 5, 2021
Merged

EIP1559 New fee support #1509

merged 37 commits into from
Aug 5, 2021

Conversation

optout21
Copy link
Contributor

@optout21 optout21 commented Jul 8, 2021

Description

New EIP1559 fee structure support. Fixes #1322.

  • One common proto for both legacy and new EIP1559 fee structure, with 2 new fields, and gasPrice=0 for EIP1559.
  • New EIP1559 Transaction type class (types), with type = 2.

Note: Ethereum proto and binary format changed.

Testing instructions

New EIP1559-fee TX, tested on Ropsten testnet.
Unit tests.

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist

  • Prefix PR title with [WIP] if necessary.
  • Add tests to cover changes as needed.
  • Update documentation as needed.
  • I have read the guidelines for adding a new blockchain
  • If there is a related Issue, mention it in the description (e.g. Fixes #<issue_number> ).

@optout21 optout21 marked this pull request as ready for review July 12, 2021 15:00
@optout21 optout21 requested a review from hewigovens as a code owner July 12, 2021 15:00
@optout21 optout21 requested a review from vikmeup July 12, 2021 15:00
@hewigovens
Copy link
Contributor

how to calculate max_inclusion_fee_per_gas and max_fee_per_gas?

@optout21
Copy link
Contributor Author

how to calculate max_inclusion_fee_per_gas and max_fee_per_gas?

I have not came across a guide targeted at wallet developers.
I think we can observe base fee (changing dynamically by the network), offer an inclusion fee (tip) which can be overridden by the user (as gas price is possible today), and compute max fee also.

src/Ethereum/Signer.cpp Show resolved Hide resolved
src/Ethereum/Signer.cpp Show resolved Hide resolved
src/Ethereum/Transaction.cpp Outdated Show resolved Hide resolved
@optout21 optout21 marked this pull request as draft July 16, 2021 11:07
@optout21
Copy link
Contributor Author

Put back to Draft, some small changes planned.

@optout21 optout21 changed the title EIP1559 New fee support [WIP] EIP1559 New fee support Jul 16, 2021
@optout21 optout21 changed the title [WIP] EIP1559 New fee support EIP1559 New fee support Jul 19, 2021
@optout21 optout21 marked this pull request as ready for review July 28, 2021 05:15
@optout21
Copy link
Contributor Author

optout21 commented Aug 5, 2021

EIP1559 has been activated on Ethereum mainnet, time to merge this.

bytes max_inclusion_fee_per_gas = 8;

// Maxinmum fee (256-bit number)
// Used only for EIP1559 fee, disregarded for legacy
Copy link
Contributor

Choose a reason for hiding this comment

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

do you think we can order properties by index? to avoid confusion in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what's the best approach:

  • change index numbers to match the order --> risk of breaking binary usage (had issues in the past)
  • reorder in the source code to match indexes --> confusing, items close logically end up separates
  • keep as it is

@optout21 optout21 merged commit 1e34cc2 into master Aug 5, 2021
SHH-HaoZhang pushed a commit to monacohq/wallet-core that referenced this pull request Oct 15, 2021
* Add new fields to Ethereum proto.

* Legacy transaction building.

* Typescript test update

* TS test fix

* Compiler warning fix (ios)

* Legacy and Enveloped transactions.

* lSigner.
Refactor Transaction

* AccessList generation test (wip)

* Cleanup, small refactor

* Signer build change, make it more generic.

* Copyright year

* SignatureRSV rename.

* Signer class is static now, no chainID memeber.

* Renames, Legacy

* Minor touches

* Rename, hash

* Minor reorg in sign()

* Eip1559 tx implementation (native transfer only).

* Additional EIP1559 build methods

* Revert renumbering in proto file (to be bw compatible)

* Additional tests for 1559-fee'd ERC20 and other 3 messages

* Test fix

* iOS test data fix

* Android test fix

* Update coverage (94.6%)

* Merge fix

* Merge fix

* Add comment on replay protection

* Empty (access) list as const.

Co-authored-by: Catenocrypt <[email protected]>
Co-authored-by: hewigovens <[email protected]>
@optout21 optout21 deleted the ar/eip1559-2 branch November 30, 2021 09:04
cornbread78 pushed a commit to cornbread78/wallet-core that referenced this pull request Dec 22, 2021
* Add new fields to Ethereum proto.

* Legacy transaction building.

* Typescript test update

* TS test fix

* Compiler warning fix (ios)

* Legacy and Enveloped transactions.

* lSigner.
Refactor Transaction

* AccessList generation test (wip)

* Cleanup, small refactor

* Signer build change, make it more generic.

* Copyright year

* SignatureRSV rename.

* Signer class is static now, no chainID memeber.

* Renames, Legacy

* Minor touches

* Rename, hash

* Minor reorg in sign()

* Eip1559 tx implementation (native transfer only).

* Additional EIP1559 build methods

* Revert renumbering in proto file (to be bw compatible)

* Additional tests for 1559-fee'd ERC20 and other 3 messages

* Test fix

* iOS test data fix

* Android test fix

* Update coverage (94.6%)

* Merge fix

* Merge fix

* Add comment on replay protection

* Empty (access) list as const.

Co-authored-by: Catenocrypt <[email protected]>
Co-authored-by: hewigovens <[email protected]>
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.

Ethereum new fee support EIP1559
3 participants