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

Merge bulk of London support to master #2060

Merged
merged 10 commits into from
Jul 8, 2021
Merged

Merge bulk of London support to master #2060

merged 10 commits into from
Jul 8, 2021

Conversation

fselmo
Copy link
Collaborator

@fselmo fselmo commented Jul 2, 2021

What was wrong?

  • London support was needed, mainly support for the new transaction parameters maxFeePerGas and maxPriorityFeePerGas

Related to Issue # 1835

How was it fixed?

  • Support maxFeePerGas and maxPriorityFeePerGas was added, mainly to the sendTransaction RPC call and internal replace_transaction and modify_transaction methods.

Todo:

- [ ] Add entry to the release notes
^ The separate commits in this branch have their appropriate release notes

Cute Animal Picture

Screen Shot 2021-07-02 at 14 48 43

@fselmo fselmo changed the title London Merge London changes to master Jul 2, 2021
@fselmo fselmo requested a review from kclowes July 2, 2021 22:01
@fselmo fselmo force-pushed the london branch 4 times, most recently from c5d3b60 to d8a6177 Compare July 3, 2021 01:26
Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

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

Hooray! We're so close!

There were a few things I noticed when manually testing:

  • I think sign_transaction still needs to be updated to support 1559. If I send a tx with both 1559 and legacy fields, I get a ValueError back from Geth, rather than our web3 TransactionTypeMismatch error
  • In the same vein, if I sign_transaction with an int in the maxFeePerGas field:
w3.eth.sign_transaction({
    'from': w3.eth.default_account, 
    'to': acct2, 
    'value': 100, 
    'maxFeePerGas': 3000000000,
    'nonce': w3.eth.get_transaction_count(w3.eth.default_account),
})

I get an invalid argument 0: json: cannot unmarshal non-string into Go struct field TransactionArgs.maxFeePerGas of type *hexutil.Big" error.

  • It's probably also worth writing a few integration tests for replace_transaction and modify_transaction to prove to ourselves that we can handle both legacy and 1559 transaction params with those two methods. A quick look suggests that we'll at least need to add the new keys to the VALID_TRANSACTION_PARAMS in web3/_utils/transactions.py but there may be other things that need to be done as well.
  • It may be worth adding a note to the docs for the Gas Price API and/or w3.eth.generate_gas_price that those will only work for legacy transactions

Thanks for all of your work on this!

@fselmo
Copy link
Collaborator Author

fselmo commented Jul 7, 2021

It's probably also worth writing a few integration tests for replace_transaction and modify_transaction to prove to ourselves that we can handle both legacy and 1559 transaction params with those two methods.

I did change most of the relevant replace_transaction tests to use the new params in this PR and kept a test_eth_replace_transaction_legacy. I left gasPrice only on the tests that are testing gas price strategies for replace_transaction. It would be good to add the params to modify_transaction though, thanks for catching that.

I'll also add some clarification on the gas price strategy to the docs 👌

And just going to mark it here that, as we saw earlier, geth's eth_signTransaction doesn't appear to be supporting the new params at the moment so we can circle back to that if this PR is still open when they implement it. Otherwise I would suggest we add that support as a separate PR.

@fselmo fselmo force-pushed the london branch 4 times, most recently from 2d94953 to 6ee262d Compare July 8, 2021 19:24
Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

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

Awesome! This looks good to me! 🚀

wolovim and others added 10 commits July 8, 2021 14:07
- Fixed tests that were failing from the implementation of the new EIP-1559 fields (maxFeePerGas and maxPriorityFeePerGas). Updated most of our existing tests to use these new fields in order to encourage their use / be updated to the new standard.
- Updated documentation around unit and integration testing and how to contribute to our test suite.
Some of our tests had 'gasPrice' set too low. This would cause those transactions to sit in the tx pool - often interfering with other tests that were either looking for a mined transaction or looking to be included in the pending block. This commit attempts to fix that and turn some tests that were previously failing back on.
Added London support for the async eth_sendTransaction calls. Also added back the gas price strategy for calls that have a gas price strategy explicitly set and do not have a 'gasPrice' set. This is mostly so we do not introduce a breaking change when it comes to setting a strategy for legacy txns.
Cleaned up some setup that we had tested against before geth released v1.10.4 with London support. Turned a unit test for gas price strategy back on now that we added back support for it.
Added support for maxFeePerGas and maxPriorityFeePerGas to the modify_transaction method. Updated most function examples throughout the docs to use maxFeePerGas and maxPriorityFeePerGas over gasPrice. Consolidated the newsfragments for EIP-1559 updates to one feature update and used the london branch PR number for the fragment.
@fselmo fselmo changed the title Merge London changes to master Merge bulk of London support to master Jul 8, 2021
@fselmo fselmo merged commit 2d89615 into master Jul 8, 2021
@fselmo fselmo deleted the london branch July 8, 2021 20:17
@qiushui777
Copy link

Hi, I was trying to sign a tx with the latest web3.py which is 5.21.0.

txn_dict = contract.functions.test(int(amount)).buildTransaction({ 'chainId': 5, 'gas': 2000000, 'maxFeePerGas': w3.toWei(7, 'gwei'), 'nonce': nonce, }) print(txn_dict) signed_txn = w3.eth.account.signTransaction(txn_dict, private_key=privatekey)

and it gave me this error

TypeError: Transaction must not include unrecognized fields: {'maxFeePerGas'}

Any idea on how to solve it? I think it should support maxFeePerGas when signing the tx

@fselmo
Copy link
Collaborator Author

fselmo commented Aug 1, 2021

Hey @qiushui777. These changes are going to be on the next release. Sorry for the inconvenience. For now you can get around this by installing web3py from the latest master branch with pip install https://github.com/ethereum/web3.py/archive/master.zip.

This next release will come out early this next week if not tomorrow.

@fselmo
Copy link
Collaborator Author

fselmo commented Aug 2, 2021

@qiushui777 Version 5.22.0 is out now and should include signing support for 1559

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.

4 participants