-
Notifications
You must be signed in to change notification settings - Fork 30
fix(purchasing): fix crash completing future more than once #1249
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
Conversation
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.
\o/
proc callback(_: RequestId) = | ||
done.complete() | ||
done.fire() |
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.
Is the callback supposed to be called more than once, if not this is probably an issue somewhere else?
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.
Agreed. This smells like a subscription callback or ethers error, or maybe the event is indeed being fired more than once in the contract.
The root issue will be masked by this change. I don't think it's a big deal personally, but it could potentially hide other issues.
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.
Agreed that it should not be called more than once. However it might be called more than once if the JSON-RPC provider decides to invoke the callback more than once for whatever reason (chain reorg?), so we'd better not crash if it does 😄
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 took a look into |
8767b59
to
587d6e9
Compare
587d6e9
to
2167a6e
Compare
Fixes #962