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

fix(relayer): estimate gas for tx, set gas to 2.5mil if not estimatable. works now. #13271

Merged
merged 3 commits into from
Mar 8, 2023

Conversation

cyberhorsey
Copy link
Contributor

No description provided.

RogerLamTd
RogerLamTd previously approved these changes Mar 7, 2023
@codecov
Copy link

codecov bot commented Mar 7, 2023

Codecov Report

Merging #13271 (b1ecddc) into main (7cd7787) will decrease coverage by 0.12%.
The diff coverage is 51.61%.

@@            Coverage Diff             @@
##             main   #13271      +/-   ##
==========================================
- Coverage   61.61%   61.50%   -0.12%     
==========================================
  Files         117      118       +1     
  Lines        3460     3468       +8     
  Branches      485      485              
==========================================
+ Hits         2132     2133       +1     
- Misses       1244     1249       +5     
- Partials       84       86       +2     
Flag Coverage Δ *Carryforward flag
bridge-ui 93.80% <ø> (ø) Carriedforward from 6954529
protocol 52.37% <ø> (ø) Carriedforward from 6954529
relayer 64.86% <51.61%> (-0.33%) ⬇️
ui 100.00% <ø> (ø) Carriedforward from 6954529

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
packages/relayer/message/process_message.go 51.40% <25.00%> (-5.17%) ⬇️
packages/relayer/message/estimate_gas.go 60.00% <60.00%> (ø)
packages/relayer/message/is_profitable.go 100.00% <100.00%> (+17.64%) ⬆️
packages/relayer/http/server.go 96.47% <0.00%> (+2.35%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@RogerLamTd
Copy link
Contributor

Just out of curiosity, is there a specific benefit to separating estimate gas and isProfitable? @cyberhorsey

@cyberhorsey
Copy link
Contributor Author

Just out of curiosity, is there a specific benefit to separating estimate gas and isProfitable? @cyberhorsey

Not only are they two different domain problems that should be unrelated, it lets us handle the errors from them separately. For instance now we don't even run the isProfitable code if the CLI flag for isProfitable isnt set, and now if the estimateGas code doesn't return us a good number, we set a large limit (because it seems unable to estimate cost for deploying an ERC20 when necessary)

@RogerLamTd
Copy link
Contributor

Just out of curiosity, is there a specific benefit to separating estimate gas and isProfitable? @cyberhorsey

Not only are they two different domain problems that should be unrelated, it lets us handle the errors from them separately. For instance now we don't even run the isProfitable code if the CLI flag for isProfitable isnt set, and now if the estimateGas code doesn't return us a good number, we set a large limit (because it seems unable to estimate cost for deploying an ERC20 when necessary)

Gotcha, so we're leaving the flag as false for this coming testnet entirely then

@cyberhorsey cyberhorsey added this pull request to the merge queue Mar 8, 2023
Merged via the queue into main with commit 3913ca5 Mar 8, 2023
@cyberhorsey cyberhorsey deleted the relayer_gas branch March 8, 2023 01:26
@github-actions github-actions bot mentioned this pull request Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants