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

Add explicit gas to exchange transactions to prevent errors #2552

Merged
merged 6 commits into from
Jan 28, 2020

Conversation

nategraf
Copy link
Contributor

Description

Add explicit gas parameter to exchange transactions, with suggested value from @mrsmkl, in the CLI to prevent an out of gas error that randomly effects CLI users as detailed by #2541

Tested

Manually running the command against a testnet about 10 times.

Related issues

Backwards compatibility

100%

@codecov
Copy link

codecov bot commented Jan 27, 2020

Codecov Report

Merging #2552 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #2552    +/-   ##
========================================
  Coverage   73.91%   73.91%            
========================================
  Files         557      557            
  Lines       13826    13826            
  Branches     1661     1425   -236     
========================================
  Hits        10219    10219            
+ Misses       3327     3326     -1     
- Partials      280      281     +1
Flag Coverage Δ
#mobile 74.1% <ø> (ø) ⬆️
#web 73.63% <ø> (ø) ⬆️
Impacted Files Coverage Δ
packages/web/src/utils/abortableFetch.ts 71.42% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea4646b...260c1ae. Read the comment docs.

@nategraf nategraf added the automerge Have PR merge automatically when checks pass label Jan 28, 2020
Copy link
Contributor

@mrsmkl mrsmkl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I think we should keep the issue open so that we remember to check that we now have enough gas.

@@ -41,6 +41,7 @@ export default class ExchangeDollars extends BaseCommand {
await displaySendTx('approve', stableToken.approve(exchange.address, sellAmount.toFixed()))

const exchangeTx = exchange.exchange(sellAmount.toFixed(), minBuyAmount!.toFixed(), false)
await displaySendTx('exchange', exchangeTx)
// Set explciit gas based on github.com/celo-org/celo-monorepo/issues/2541
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spelling.

@celo-ci-bot-user celo-ci-bot-user merged commit 1f62db6 into master Jan 28, 2020
@celo-ci-bot-user celo-ci-bot-user deleted the victor/exchange-gas branch January 28, 2020 15:04
aaronmgdr added a commit that referenced this pull request Jan 28, 2020
* master:
  🧹Web cleanup (readme + static dir) (#2562)
  Add readable proposals to governance:view command (#2545)
  Add explicit gas to exchange transactions to prevent errors (#2552)
  Fix off-by-one error in attributing signatures to blocks in CLI (#2559)
  ✅ add test for phone Input component (#2554)
  Add Youtube to Footer +  (#2556)
  Fix rounding error in Election.sol (#2540)
  [Wallet] Bump @celo/client to 0.0.266 (#2551)
  [Wallet] E2E test improvements (#2542)
  Deployed integration (#2550)
  do not fetch affiliates (#2508)
  Added more CLI checks for registering validators and groups (#2491)
  Micro Improvement to web tests (#2527)
  [Wallet] Prompt users with connectivity issues to switch to forno (#2526)
  cli: Fix voter rewards presentation (#2543)
  [Wallet] Fix missing spanish translation  (#2539)
  Downtime slashing when epoch changes (#2436)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Have PR merge automatically when checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants