-
Notifications
You must be signed in to change notification settings - Fork 60
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
Gas estimation #1726
base: develop
Are you sure you want to change the base?
Gas estimation #1726
Conversation
✅ Deploy Preview for swapr ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
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.
Left some comments. Code looks good just needs a few improvements.
src/hooks/useSwapCallback.ts
Outdated
let transactionRequest: any | ||
try { | ||
transactionRequest = await call.transactionParameters | ||
} catch (e) { | ||
console.log('Failed to get transaction parameters', e) | ||
} | ||
|
||
// Ignore gas estimation if the request has gasLimit property | ||
if (transactionRequest.gasLimit) { |
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.
If transactionRequest
is undefined it could break when checking transactionRequest.gasLimit
. Also I don't think we want to console.log
the error, use .error
in this case.
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.
Yeah this part I noticed during testing that it wasn't handling the error so I put this in place. It didn't throw it for me.
Tested and looks ok to me 👍 |
fb69230
to
0c5053c
Compare
ff47650
to
b3b3815
Compare
91e4f23
to
af06da1
Compare
Linked to this sdk pr
SwaprHQ/swapr-sdk#276
To test:
Some notes from my research: