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

accounts/abi, mobile: use EIP155Signer for new txs (#16484) #17046

Closed
wants to merge 1 commit into from

Conversation

dinstein
Copy link

Fix #16484.

@dinstein dinstein requested a review from karalabe as a code owner June 21, 2018 06:50
@karalabe
Copy link
Member

I'm a bit uncertain about this change. If we have the SignerFn already supplied from the outside, why not add the EIP155 changes to that instead? This PR feels as if we're leaking out logic into the wrong place.

@dinstein
Copy link
Author

dinstein commented Jun 26, 2018

I thought the ChainId may be an attribute of transaction (just like Nonce) after EIP155 hard fork, so I added it into TransactOpts. While if we treat it as an attribute of SignFn, indeed it should not be included in TransactOpts.

SignFn requires a types.Signer input parameter, but without ChainId we could not create an EIP155 Signer in transact function either pass it to SignFn, so shall we implement a SignFn that ignore the first parameter types.Signer and create an EIP155 Signer inside instead?

@dinstein
Copy link
Author

As shown in mobile/bind.go, the SetSigner create an opts.Signer which ignores the input parameter signer and uses mobile.Signer to sign a txn. I think this may be a little confusing, if we want SignFn to handle all the signing actions, maybe we should remove its first parameter types.Signer and only pass address and unsigned txn to it.

@Dominator008
Copy link

What's the status of this?

@gballet
Copy link
Member

gballet commented May 20, 2019

@dinstein you are right, having the signer as the first parameter leads to a confusing situation. Removing the first parameter is a good idea. It will require a change in NewKeyedTransactor but it's easily done with that SetSigner function.

@dinstein
Copy link
Author

@gballet glad to see the progress :)

@stale
Copy link

stale bot commented May 30, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot closed this Jul 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Abi.bind should use EIP155Signer (not HomesteadSigner) for new transactions.
5 participants