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

accounts/abi/bind: allow to specify signer on transactOpts #21356

Merged
merged 8 commits into from
Dec 8, 2020
Merged

accounts/abi/bind: allow to specify signer on transactOpts #21356

merged 8 commits into from
Dec 8, 2020

Conversation

MariusVanDerWijden
Copy link
Member

@MariusVanDerWijden MariusVanDerWijden commented Jul 22, 2020 •
edited
Loading

This commit enables users to specify which signer they want to use while creating their transactOpts.
Previously all contract interactions used the homestead signer. Now a user can specify whether they
want to sign with homestead or EIP155 and specify the chainID which adds another layer of security.

closes #16484

This PR changes the interface of New{}Transactor, so it is a breaking change for everyone who uses the bind package to create their signer.

@karalabe
Copy link
Member

Wondering if we should use a chainid instead of a signer as a new parameter? The auth package is mostly a convenience thing so you don't need to construct your transactor manually and so you don't need to care about the details. It seems sane to me to explicitly enforce EIP155 in a convenience method and let the user do the manual dance if they want he unsecure version. It's also easier to tell the users to pass the chainid, than to "construct an EIP155 signer".

What are your thoughts? I'm open to alternatives, just looking at it from an API perspective.

Also this PR would break all existing code relying on abigen generated contracts :P

@MariusVanDerWijden
Copy link
Member Author

Yeah I think that might be a better interface, we don't really want to encourage homestead signers anymore, right?
I'll change it to use ChainID and eip155 signers.
About breaking all existing contracts: Yeah but I think the change might be so useful that it outweighs the inconveniences.
Only problem I can see is that Dapp developers have to introduce a global variable to identify the network, default it to 1 and set it to 1337 in testing. Oh btw It is not really documented that all simulated backends run chains with chainID 1337 in the source code, shall I add some docu there?

@karalabe
Copy link
Member

The more docs the better :)

holiman
holiman approved these changes Sep 1, 2020
View reviewed changes
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

Makes sense to me, but I'm not really well versed in that whole abi/bind thingy, I've never used it and am not the best person to give a thumbs-up :)

gballet
gballet requested changes Sep 1, 2020
View reviewed changes
Copy link
Member

@gballet gballet left a comment

Choose a reason for hiding this comment

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

In order not to break contracts right off the bat, It should be possible to keep the legacy system and display a deprecation warning when the function is used, and offer the new API with a slightly different name. Leave it like that for a few releases and then remove it.

@MariusVanDerWijden
Copy link
Member Author

Hmm I restored the old functions now, I'm not a huge fan, but at least I'll get a purely red pr in some months

gballet
gballet approved these changes Sep 7, 2020
View reviewed changes
Copy link
Member

@gballet gballet left a comment

Choose a reason for hiding this comment

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

LGTM

fjl
fjl requested changes Sep 10, 2020
View reviewed changes
accounts/abi/bind/auth.go Outdated Show resolved Hide resolved
karalabe
karalabe reviewed Sep 29, 2020
View reviewed changes
accounts/abi/bind/auth.go Outdated Show resolved Hide resolved
@MariusVanDerWijden
Copy link
Member Author

cc @fjl

@MariusVanDerWijden
internal/ethapi: add optional parameter blockNrOrHash to estimateGas (e…
7a68ffd 
…thereum#21545)

This allows users to estimate gas on top of arbitrary blocks as well as pending and latest.
Tracing on pending is useful for most users as it takes into account the current txpool while
tracing on latest might be useful for users that have little to know knowledge of the current
transactions in the network.

If blockNrOrHash is not specified, estimateGas defaults to pending
@MariusVanDerWijden
accounts/abi/bind: allow to specify signer on transactOpts
7726cde 
This commit enables users to specify which signer they want to use while creating their transactOpts.
Previously all contract interactions used the homestead signer. Now a user can specify whether they
want to sign with homestead or EIP155 and specify the chainID which adds another layer of security.
@MariusVanDerWijden
accounts/abi/bind: change api of transactor helpers
90744e1
@MariusVanDerWijden
accounts/abi/bind/: restore and deprecate previous functions
5b5241a
@MariusVanDerWijden
accounts/abi/bind: Use log.Warn instead of println
7b29c7d
@MariusVanDerWijden
accounts/abi/bind: add custom errors, don't sign for mainnet
fc7fbb4
@MariusVanDerWijden
accounts/abi/bind: better error handling
e61c432
@MariusVanDerWijden
Copy link
Member Author

cc @fjl

@quorumbot quorumbot mentioned this pull request Jun 29, 2021
Merged
14 tasks
@MariusVanDerWijden MariusVanDerWijden deleted the eip155signer branch November 30, 2021 15:24
This was referenced Sep 23, 2022
Closed
Merged
@gzliudan gzliudan mentioned this pull request May 30, 2024
Merged
19 tasks
@gzliudan gzliudan mentioned this pull request Jun 17, 2024
Merged
19 tasks
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jun 20, 2024
@gzliudan
accounts/abi/bind: allow to specify signer on transactOpts (ethereum#…
e72f6de 
…21356)
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 7, 2025
@gzliudan
accounts/abi/bind: allow specifying signer on transactOpts (ethereum#…
b9cac9a 
…21356)
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 9, 2025
@gzliudan
accounts/abi/bind: allow specifying signer on transactOpts (ethereum#…
5c21990 
…21356)
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 10, 2025
@gzliudan
accounts/abi/bind: allow specifying signer on transactOpts (ethereum#…
a80864a 
…21356)
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 10, 2025
@gzliudan
accounts/abi/bind: allow specifying signer on transactOpts (ethereum#…
863d262 
…21356)
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 10, 2025
@gzliudan
accounts/abi/bind: allow specifying signer on transactOpts (ethereum#…
67d052e 
…21356)
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 10, 2025
@gzliudan
accounts/abi/bind: allow specifying signer on transactOpts (ethereum#…
6b5fae7 
…21356)
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 14, 2025
@gzliudan
accounts/abi/bind: allow specifying signer on transactOpts (ethereum#…
6f200f9 
…21356)
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 16, 2025
@gzliudan
accounts/abi/bind: allow specifying signer on transactOpts (ethereum#…
5f457bc 
…21356)
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 17, 2025
@gzliudan
accounts/abi/bind: allow specifying signer on transactOpts (ethereum#…
ba03b7b 
…21356)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karalabe karalabe karalabe left review comments

@fjl fjl fjl requested changes

@gballet gballet gballet approved these changes

@holiman holiman holiman approved these changes

@ligi ligi Awaiting requested review from ligi

@rjl493456442 rjl493456442 Awaiting requested review from rjl493456442

@zsfelfoldi zsfelfoldi Awaiting requested review from zsfelfoldi

@renaynay renaynay Awaiting requested review from renaynay

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Abi.bind should use EIP155Signer (not HomesteadSigner) for new transactions.
7 participants
@MariusVanDerWijden @karalabe @fjl @holiman @gballet @adamschmideg @rjl493456442