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.

Sign up for GitHub

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

Jump to bottom

Don't require chainId if common is present [eth.accounts.signTransaction] #4208

Closed
Mr-VIT opened this issue Jul 29, 2021 · 1 comment · Fixed by #4293
Closed

Don't require chainId if common is present [eth.accounts.signTransaction] #4208

Mr-VIT opened this issue Jul 29, 2021 · 1 comment · Fixed by #4293
Assignees
@jdevcs
Labels
1.x 1.0 related issues Bug Addressing a bug Investigate P2 Medium severity bugs

Comments

@Mr-VIT
Copy link

Mr-VIT commented Jul 29, 2021

https://github.com/ChainSafe/web3.js/blob/458a47af814982df6cfd111c0d8a8f9b17581937/packages/web3-eth-accounts/src/index.js#L266-L267
transaction.chainId is not used anyway if transaction.common is present:
https://github.com/ChainSafe/web3.js/blob/458a47af814982df6cfd111c0d8a8f9b17581937/packages/web3-eth-accounts/src/index.js#L185-L192
@luu-alex luu-alex added the 1.x 1.0 related issues label Jul 29, 2021
@GregTheGreek
Copy link
Contributor

This makes sense, we should probably just update the chainId within the if block on line 186

@philknows philknows added P2 Medium severity bugs Bug Addressing a bug labels Aug 3, 2021
@jdevcs jdevcs self-assigned this Aug 23, 2021
@jdevcs jdevcs mentioned this issue Sep 6, 2021
Merged
13 tasks
@jdevcs jdevcs linked a pull request Sep 6, 2021 that will close this issue
add tx.chainId if tx.common.chainId not provided #4293
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jdevcs jdevcs

Labels
1.x 1.0 related issues Bug Addressing a bug Investigate P2 Medium severity bugs
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

add tx.chainId if tx.common.chainId not provided web3/web3.js
5 participants
@Mr-VIT @GregTheGreek @luu-alex @philknows @jdevcs