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

Adding a contract to store minimum required client version #1081

Merged
merged 21 commits into from
Oct 8, 2019

Conversation

mrsmkl
Copy link
Contributor

@mrsmkl mrsmkl commented Sep 23, 2019

Description

This is a smart contract where the variables can be changed using governance. As a first variable, we have the minimum client version. In celo-blockchain PR, the client will check this variable and will exit if the required version on the blockchain is higher than current version.

Tested

There is a unit test for the contract, not sure where to add a test where geth exits.

Other changes

Changed migration numbering.

Related issues

Backwards compatibility

@codecov
Copy link

codecov bot commented Sep 24, 2019

Codecov Report

Merging #1081 into master will increase coverage by 0.11%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1081      +/-   ##
==========================================
+ Coverage   66.47%   66.59%   +0.11%     
==========================================
  Files         262      257       -5     
  Lines        7648     7394     -254     
  Branches      509      430      -79     
==========================================
- Hits         5084     4924     -160     
+ Misses       2462     2375      -87     
+ Partials      102       95       -7
Flag Coverage Δ
#mobile 66.59% <ø> (+0.11%) ⬆️
Impacted Files Coverage Δ
packages/mobile/src/import/saga.ts 65.38% <0%> (-26.29%) ⬇️
packages/mobile/src/localCurrency/selectors.ts 57.14% <0%> (-12.43%) ⬇️
packages/mobile/src/localCurrency/saga.ts 65.38% <0%> (-5.59%) ⬇️
...kages/mobile/src/transactions/TransferFeedItem.tsx 80.89% <0%> (-2.74%) ⬇️
packages/mobile/src/utils/formatting.ts 87.5% <0%> (-2.25%) ⬇️
packages/mobile/src/qrcode/utils.ts 72.5% <0%> (-1.86%) ⬇️
packages/mobile/src/geth/geth.ts 24.07% <0%> (-1.86%) ⬇️
packages/mobile/src/recipients/RecipientPicker.tsx 71.62% <0%> (-1.46%) ⬇️
packages/mobile/src/transactions/reducer.ts 65.38% <0%> (-1.29%) ⬇️
packages/mobile/src/tokens/saga.ts 91.48% <0%> (-1.11%) ⬇️
... and 44 more

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 d05e4c5...2d48c4c. Read the comment docs.

Copy link
Contributor

@m-chrzan m-chrzan left a comment

Choose a reason for hiding this comment

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

Looks good. Just an ask around e2e tests and some style fixes. Here's the Solidity style guide to fix the function declarations.

@m-chrzan m-chrzan assigned mrsmkl and unassigned m-chrzan Sep 26, 2019
@mrsmkl mrsmkl assigned timmoreton and asaj and unassigned mrsmkl Sep 30, 2019
Copy link
Contributor

@asaj asaj left a comment

Choose a reason for hiding this comment

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

It looks like the 16_elect_validators migration was accidentally renamed to 51_elect_validators

@asaj asaj assigned mrsmkl and unassigned asaj Oct 2, 2019
@mrsmkl
Copy link
Contributor Author

mrsmkl commented Oct 2, 2019

I renamed the migrations because they are the ones that have to run last.

@mrsmkl mrsmkl assigned asaj and unassigned mrsmkl Oct 3, 2019
@asaj
Copy link
Contributor

asaj commented Oct 3, 2019

Rather than rename it to 51, can we keep the sequential naming?

@asaj asaj assigned mrsmkl and unassigned asaj Oct 3, 2019
Copy link
Contributor

@asaj asaj left a comment

Choose a reason for hiding this comment

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

Exciting!

* @dev For example if the version is 1.9.2, 1 is the major version, 9 is minor,
* and 2 is the patch level.
*/
function setMinimumClientVersion(uint256 major, uint256 minor, uint256 patch) public onlyOwner {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be external rather than public?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I call it from the initializer, that's why it is public...

packages/protocol/lib/registry-utils.ts Outdated Show resolved Hide resolved
packages/protocol/lib/registry-utils.ts Outdated Show resolved Hide resolved
packages/protocol/migrations/16_governance.ts Outdated Show resolved Hide resolved
@mrsmkl mrsmkl added the automerge Have PR merge automatically when checks pass label Oct 8, 2019
@ashishb ashishb merged commit ff03623 into celo-org:master Oct 8, 2019
aaronmgdr added a commit that referenced this pull request Oct 8, 2019
* master: (35 commits)
  [Wallet] Fix top of emojis cut off in the activity feed (#1243)
  Adding a contract to store minimum required client version (#1081)
  Revert "Feature #909 proxy delegatecall (#1152)" (#1241)
  Use ContractKit to get addresses for Blockchain API (#1175)
  Feature #909 proxy delegatecall (#1152)
  Fix Faucet done message (#1217)
  Updated SETUP.md with new yarn process (#1224)
  Adding `increaseAllowance` and `decreaseAllowance` methods (#1196)
  extracting function signatures (#1061)
  Fix integration hardcode (#1208)
  Fixing flaky governance test (#1155)
  Restore CI branch (#1223)
  [wallet] e2e back to green (#1210)
  [Wallet] Implement new import wallet flow designs (#1209)
  [Wallet] Fix disable conditions for butons on Enter Invite screen (#1214)
  [protocol] Rename infrastructureFraction to proposerFraction (#1174)
  [ck] Proper promise treatment to avoid UnhandledPromises (#1219)
  [ck] Transform StableToken parameters from fixidity format (#1218)
  [wallet]Store encrypted local signing key (#1188)
  2019-10-03 alfajores deployment (#1200)
  ...
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.

Blockchain SBAT reference governable smart contract with minimum client version
5 participants