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

CLI relock fix #2464

Merged
merged 3 commits into from
Jan 21, 2020
Merged

CLI relock fix #2464

merged 3 commits into from
Jan 21, 2020

Conversation

mrsmkl
Copy link
Contributor

@mrsmkl mrsmkl commented Jan 17, 2020

Description

Just added a check to make sure that locked value is not zero.

Tested

Added unit test.

Other changes

Trying to remove some flakiness from governance unit tests.

Related issues

Backwards compatibility

@mrsmkl mrsmkl requested review from asaj and mcortesi as code owners January 17, 2020 09:22
@mrsmkl mrsmkl changed the title added check to make sure that locked values is not zero CLI relock fix Jan 17, 2020
@codecov
Copy link

codecov bot commented Jan 17, 2020

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2464   +/-   ##
=======================================
  Coverage   74.53%   74.53%           
=======================================
  Files         280      280           
  Lines        7920     7920           
  Branches     1015     1015           
=======================================
  Hits         5903     5903           
  Misses       1903     1903           
  Partials      114      114
Flag Coverage Δ
#mobile 74.53% <ø> (ø) ⬆️

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 d7004fe...707b76f. Read the comment docs.

@mrsmkl mrsmkl requested a review from m-chrzan as a code owner January 17, 2020 17:50
Comment on lines +80 to +84
if (bn % EPOCH < 50) {
await mineBlocks(EPOCH + 20, web3)
} else {
await mineBlocks(EPOCH - 20, web3)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the rationale behind the +20 / -20?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well +20 is probably not useful, but -20 makes sure that the epoch won't change twice. Perhaps still need different handling for the case bn % EPOCH == 0.

Copy link
Contributor

@yerdua yerdua left a comment

Choose a reason for hiding this comment

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

I'm curious about a thing, but it's not blocking.

@yerdua yerdua merged commit b689f90 into celo-org:master Jan 21, 2020
aaronmgdr added a commit that referenced this pull request Jan 21, 2020
* master:
  [Wallet] Historical currency conversions in the transaction feed (#2446)
  CLI relock fix (#2464)
  Update copyright year + inline button  (#2468)

# Conflicts:
#	yarn.lock
aaronmgdr added a commit that referenced this pull request Jan 21, 2020
* master: (25 commits)
  Add react-testing-utils + Fix Analytics (#2437)
  collect coverage on web (#2415)
  Add callouts to serve text messages in regions (#2458)
  [Wallet] Historical currency conversions in the transaction feed (#2446)
  CLI relock fix (#2464)
  Update copyright year + inline button  (#2468)
  Voting bot for stake off (#2327)
  Change order of profile info (#2454)
  [Wallet] Fix type check regression for components wrapped by our custom `withTranslation` (#2457)
  Unfreeze rewards by default (#2452)
  Baklava and baklavastaging deploys (#2421)
  Fix coin colors  (#2441)
  Make governance CLI more usable (#2428)
  Slashing params for stake off phase 2 (#2418)
  [Wallet] Rollback zeroSync toggle in case it was not successful (#2434)
  [Wallet] Cleanup unused StateProps references (#2439)
  Add unit tests and cli checks for validator hotfix interaction (#2340)
  Add proper Spanish translations (#2427)
  Catch exceptions during polling (#2432)
  Add unfreeze-contracts command to celotool (#2433)
  ...

# Conflicts:
#	packages/web/src/brandkit/common/MobileMenu.test.tsx
#	yarn.lock
aaronmgdr added a commit that referenced this pull request Jan 21, 2020
* master:
  Add react-testing-utils + Fix Analytics (#2437)
  collect coverage on web (#2415)
  Add callouts to serve text messages in regions (#2458)
  [Wallet] Historical currency conversions in the transaction feed (#2446)
  CLI relock fix (#2464)
  Update copyright year + inline button  (#2468)
  Voting bot for stake off (#2327)
  Change order of profile info (#2454)
  [Wallet] Fix type check regression for components wrapped by our custom `withTranslation` (#2457)
  Unfreeze rewards by default (#2452)

# Conflicts:
#	packages/web/src/analytics/analytics.test.ts
#	packages/web/src/brandkit/common/MobileMenu.test.tsx
#	yarn.lock
aaronmgdr added a commit that referenced this pull request Jan 22, 2020
* master:
  add celo.org/about#contributors id (#2488)
  [Wallet] Fix integration build firebase db url on Android (#2495)
  Fixes governance CLI bugs encountered while running election contract upgrade (#2482)
  Add validator:signed-blocks command and fix validator:status bug (#2456)
  Bump @celo/celocli version to 0.0.34 (#2420)
  Fix Logo + backers number change (#2453)
  Improve election efficiency by short circuiting (#2475)
  Page for Experience / BrandKit / Composition (#2462)
  Configure the time to wait for text messages (#2450)
  Add react-testing-utils + Fix Analytics (#2437)
  collect coverage on web (#2415)
  Add callouts to serve text messages in regions (#2458)
  [Wallet] Historical currency conversions in the transaction feed (#2446)
  CLI relock fix (#2464)
  Update copyright year + inline button  (#2468)
  Voting bot for stake off (#2327)
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.

Relock throws an error but succeeds
3 participants