-
Notifications
You must be signed in to change notification settings - Fork 18
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
Fix tx nonce mismatch error: approach 1 #134
Fix tx nonce mismatch error: approach 1 #134
Conversation
532632c
to
889ef0d
Compare
LGTM, but there is a typo in your sign off. |
889ef0d
to
e086a22
Compare
Fixed the signed-off message. |
@@ -130,6 +143,17 @@ func (c *ContractBackend) NewTransactor(ctx context.Context, gasLimit uint64, | |||
auth.GasLimit = gasLimit | |||
auth.Context = ctx | |||
|
|||
nonce, err := c.PendingNonceAt(ctx, acc.Address) | |||
if err != nil { | |||
return nil, errors.Wrap(err, "fetching nonce") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you have to check for IsChainNotReachableError
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matthiasgeihs Because, we explicitly inform the user by returning a named error when there are problems connecting with the blockchain node.
@ggwpez I have fixed this now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. It seems a bit unclean that we introduce such specific error handling logic in so many places. We could add wrappers for the handful of functions that need this handling and then consistently use the wrappers, right? It's ok here, but it's something worth discussing in general, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes the initial approach calls the error wrapping on the lowest level, where we also could have done it on the highest…
Its much more effort like this i think.
e086a22
to
a508719
Compare
- Previously, ERC20 deposit transactions would not work when using ganache-cli node. Because ERC20 deposit involved two txs and both txs got the same nonce. - Now, track the nonce in contract backend for each account and use it to ensure no two transactions get the same nonce. Resolves hyperledger-labs#62. Signed-off-by: Manoranjith <ponraj.manoranjitha@in.bosch.com>
a508719
to
402b9e5
Compare
Fix is attempted using approach 1 suggested in #62.