Versioning of contracts and upgradability test by vzotova · Pull Request #1470 · nucypher/nucypher

vzotova · 2019-11-22T12:23:53Z

Closes #1464
Closes #1465

Version for upgradeable contracts inside contract doc (@dev), for example |v1.2.3|
Multi-source solidity compilation
Extracting version inside SolidityCompiler
Storing version in contract registry
Using version while deployment
Test for upgrading possibility between current commit and master

Version format is |vi.j.k|, where i - fork (I hope will be always 1), j - upgradeable signature/state changes, k - small changes inside existent function

codecov · 2019-11-22T17:36:39Z

Codecov Report

Merging #1470 into master will decrease coverage by 0.08%.
The diff coverage is 91.86%.

@@            Coverage Diff             @@
##           master    #1470      +/-   ##
==========================================
- Coverage   84.31%   84.22%   -0.09%     
==========================================
  Files          72       72              
  Lines       11225    11303      +78     
==========================================
+ Hits         9464     9520      +56     
- Misses       1761     1783      +22

Impacted Files	Coverage Δ
nucypher/blockchain/eth/agents.py	`91.15% <ø> (ø)`	⬆️
nucypher/blockchain/eth/actors.py	`86.95% <100%> (ø)`	⬆️
nucypher/utilities/sandbox/blockchain.py	`79.22% <100%> (ø)`	⬆️
nucypher/blockchain/eth/deployers.py	`89.35% <100%> (+0.86%)`	⬆️
nucypher/cli/deploy.py	`83.41% <100%> (+0.15%)`	⬆️
nucypher/cli/painting.py	`80.11% <100%> (ø)`	⬆️
nucypher/blockchain/eth/interfaces.py	`72.12% <82.75%> (+0.99%)`	⬆️
nucypher/blockchain/eth/registry.py	`83.38% <92.85%> (-0.01%)`	⬇️
nucypher/blockchain/eth/sol/compile.py	`84.94% <93.33%> (+4.94%)`	⬆️
nucypher/characters/lawful.py	`87.47% <0%> (-2.21%)`	⬇️
... and 4 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 61f749e...5680bd2. Read the comment docs.

KPrasch · 2019-12-03T22:04:36Z

nucypher/blockchain/eth/sol/compile.py

+            raw_interfaces = self._compile(root_source_dir, other_source_dirs)
+            for name, data in raw_interfaces.items():
+                # Extract contract version from docs
+                version_search = re.search(r'\"details\":\".*?\|(v\d+\.\d+\.\d+)\|.*?\"', data['devdoc'])


Can you expand this regex into the verbose format?

Example:

p = re.compile(r""" (\+\d{1,2})? # Optional Country Code +1 or perhaps +22 (-|$)? # Optional hyphen or open parenths for the area code. (\d{10} # Try and match ten digits exactly | # Else (\d{3} # Match three digits [\-|$]{0,2}) # Hypen or close parenths zero through 2 times (to account for -)) {2} # Repeat to match the next three digits \d{4}) # The remaining four digits $ # Ensure nothing else follows """, re.VERBOSE)

KPrasch · 2019-12-03T22:05:46Z

nucypher/blockchain/eth/sol/source/contracts/staking_contracts/StakingInterface.sol

    * @param _index Index of the sub stake
    * @param _periods Amount of periods for extending sub stake
-    **/
+    */


KPrasch · 2019-12-07T02:59:18Z

nucypher/blockchain/eth/sol/compile.py

            for name, data in raw_interfaces.items():
                # Extract contract version from docs
-                version_search = re.search(r'\"details\":\".*?\|(v\d+\.\d+\.\d+)\|.*?\"', data['devdoc'])
+                version_search = re.search(r"""


👍 Badass

KPrasch · 2019-12-11T20:31:08Z

tests/blockchain/eth/contracts/integration/test_contracts_upgradability.py

@@ -0,0 +1,144 @@
+"""


🚀 - This is a relief, good work. I think this test can be emphasized in our CI workflow by running it in its own job.

KPrasch · 2019-12-11T20:33:11Z

tests/blockchain/eth/entities/deployers/test_interdeployer_integration.py

    with pytest.raises(BaseContractDeployer.ContractDeploymentError):
        assert policy_manager_deployer.contract_address is constants.CONTRACT_NOT_DEPLOYED
-    assert not policy_manager_deployer.is_deployed
+    assert not policy_manager_deployer.is_deployed()


Is PolicyManagerDeployer.is_deployed no longer a @property method?

I added version as parameter to this method, None by default means any version of contract

KPrasch · 2019-12-12T00:35:43Z

nucypher/cli/deploy.py

 @click.option('--retarget', '-d', help="Retarget a contract's proxy.", is_flag=True)
 @click.option('--target-address', help="Recipient's checksum address for token or ownership transference.",
              type=EIP55_CHECKSUM_ADDRESS)
+@click.option('--ignore-deployed', help="Ignore already deployed contracts if exist.", is_flag=True)


return of "bare" mode? I like your name better.

It's related to "bare" but much simpler, turns off 'is_deployed' check to redeploy same contract multiple times (first of all for test)

Just for my own understanding: is this flag essentially a force type option to deploy the contract even if previously deployed?

Correct, one remark - force for the same contract with the same version

Does this have a use beyond tests?

I see at least one use: redeploying same contract but with different constructor parameters, for example, disable/enable test mode of StakingEscrow (for worklock)

But are we going to do that on a real deployment? because as far as I understand, the test mode is only for tests. Or are you planning on trying that on testnet?

test mode for testnet, to change worklock

KPrasch · 2019-12-12T00:38:29Z

tests/blockchain/eth/interfaces/test_solidity_compiler.py

+    ])
+    interfaces = solidity_compiler.compile()
+
+    # Remove AST because id in tree node depends on compilation scope


Woah - can you expand a bit on this?

Each element in AST has ID field. This value depends on scope of compiled files (all contracts). First source for SolidityCompiler includes only main contracts, second - also tests. Merging two results lead that main contracts has AST from first compilation, test contracts from the second. Interface has contracts with the same AST as in second compilation for all contracts. We don't use AST yet and main part for comparing is bytecode, so I remove it

derekpierre

🎸 - Very organized and clean! Added some comments, but I'm unsure I'm knowledgeable enough about this part of the codebase to add a meaningful approval - best for me to leave it to the dev pros 😄

docs/source/architecture/contracts.md

derekpierre · 2019-12-18T15:18:21Z

nucypher/cli/deploy.py

 @click.option('--retarget', '-d', help="Retarget a contract's proxy.", is_flag=True)
 @click.option('--target-address', help="Recipient's checksum address for token or ownership transference.",
              type=EIP55_CHECKSUM_ADDRESS)
+@click.option('--ignore-deployed', help="Ignore already deployed contracts if exist.", is_flag=True)


Just for my own understanding: is this flag essentially a force type option to deploy the contract even if previously deployed?

Co-Authored-By: Derek Pierre <derek.pierre@gmail.com>

cygnusv · 2019-12-19T12:18:11Z

nucypher/blockchain/eth/interfaces.py

+                requested_version == 'earliest' and \
+                (major < current_version_parsed[0] or
+                 major == current_version_parsed[0] and minor < current_version_parsed[1] or
+                 major == current_version_parsed[0] and minor == current_version_parsed[1] and patch < current_version_parsed[2]):


Wow, a 9-line condition. This is though!
I think this part needs some touches to improve readability. As far as I can tell, you're trying to check lexicographical ordering between tuples, which Python implements already. So, e.g., lines 667 to 669 can be changed to (major, minor, patch) > current_version_parsed; similarly, lines 671 to 673 can be changed to (major, minor, patch) < current_version_parsed

@cygnusv That's pretty cool - didn't know you could do that.

Thanks, really cool suggestion.
✔️

cygnusv · 2019-12-19T12:35:11Z

nucypher/cli/deploy.py

 @click.option('--retarget', '-d', help="Retarget a contract's proxy.", is_flag=True)
 @click.option('--target-address', help="Recipient's checksum address for token or ownership transference.",
              type=EIP55_CHECKSUM_ADDRESS)
+@click.option('--ignore-deployed', help="Ignore already deployed contracts if exist.", is_flag=True)


Does this have a use beyond tests?

Co-Authored-By: Derek Pierre <derek.pierre@gmail.com> Co-Authored-By: David Núñez <david@nucypher.com>

cygnusv

Awesome work, @vzotova ! As usual :)
My only strong comment is regarding the CircleCI configuration, where I really think it should be restored to its original state.

cygnusv · 2019-12-24T15:01:13Z

.circleci/config.yml

+      - run:
+          name: Ethereum Contract Upgradeability Test
+          command: |
+            pytest $(circleci tests glob "tests/blockchain/eth/contracts/test_contracts_upgradeability.py" | circleci tests split --split-by=timings)


After checking the performance of the CI build, I think having a separate job for this test is unnecessary. It blocks a CI box for some time, while if it were included in the contracts job, it will probably be executed without any overhead due to current parallelism.

The main reason for extracting was readability of test results, like in 1459, where only one contract test failed, I like how it looks

Right, I understand, but we have a limited number of CI boxes and using one of them for a single test that runs for 21 seconds doesn't seem a good idea to me.

makes sense, I'll change it back

cygnusv · 2019-12-24T15:03:46Z

nucypher/blockchain/eth/sol/compile.py

            for name, data in raw_interfaces.items():
                # Extract contract version from docs
-                version_search = re.search(r'\"details\":\".*?\|(v\d+\.\d+\.\d+)\|.*?\"', data['devdoc'])
+                version_search = re.search(r"""


cygnusv · 2019-12-24T15:04:35Z

nucypher/blockchain/eth/sol/source/contracts/Issuer.sol

 /**
 * @notice Contract for calculate issued tokens
-**/
+* @dev |v1.1.2|


What about considering v.1.1.1 for all contracts?

I just tried to remember how many changes we did before, but really it doesn't matter because of testnet redeploy and new versions v2.1.1

cygnusv · 2019-12-24T15:09:31Z

nucypher/blockchain/eth/deployers.py

-    @property
-    def is_deployed(self) -> bool:
-        return bool(self._contract is not CONTRACT_NOT_DEPLOYED)
-


Yes! I had a very similar change in a WIP PR. This property was just checking if the contract was deployed with that very same instance of the deployer, which of course is not very useful in use cases like upgrades or rollbacks. Glad this is changed :)

…move parameters from constructor in deployers (overrides)

Co-Authored-By: Derek Pierre <derek.pierre@gmail.com> Co-Authored-By: David Núñez <david@nucypher.com>

vzotova added Enhancement New or improved features ETH Contracts Test 🔍 Tests only labels Nov 22, 2019

vzotova self-assigned this Nov 22, 2019

vzotova force-pushed the upgradability-test branch 4 times, most recently from 468fdc1 to 5035026 Compare November 29, 2019 11:34

vzotova force-pushed the upgradability-test branch 2 times, most recently from c545440 to 8458dfc Compare December 3, 2019 17:08

vzotova changed the title ~~[WIP] Versioning of contracts and upgradability test~~ Versioning of contracts and upgradability test Dec 3, 2019

vzotova marked this pull request as ready for review December 3, 2019 17:16

vzotova requested review from KPrasch, cygnusv, michwill, mswilkison and tuxxy December 3, 2019 17:20

KPrasch reviewed Dec 3, 2019

View reviewed changes

vzotova force-pushed the upgradability-test branch from 8458dfc to 851de8f Compare December 5, 2019 10:15

vzotova added a commit to vzotova/nucypher that referenced this pull request Dec 5, 2019

Apply suggestions from code review nucypher#1470

3c12856

vzotova added a commit to vzotova/nucypher that referenced this pull request Dec 6, 2019

Apply suggestions from code review nucypher#1470

5680bd2

vzotova force-pushed the upgradability-test branch from 3c12856 to 5680bd2 Compare December 6, 2019 19:14

KPrasch reviewed Dec 7, 2019

View reviewed changes

vzotova mentioned this pull request Dec 7, 2019

Policy refactoring #1459

Merged

KPrasch reviewed Dec 11, 2019

View reviewed changes

cygnusv added this to the Testnet v2 milestone Dec 11, 2019

KPrasch reviewed Dec 12, 2019

View reviewed changes

vzotova added a commit to vzotova/nucypher that referenced this pull request Dec 12, 2019

Apply suggestions from code review nucypher#1470

0b92d15

vzotova force-pushed the upgradability-test branch from 5680bd2 to 0b92d15 Compare December 12, 2019 07:45

michwill approved these changes Dec 12, 2019

View reviewed changes

vzotova mentioned this pull request Dec 18, 2019

Re-stake flag invert #1542

Merged

derekpierre reviewed Dec 18, 2019

View reviewed changes

vzotova added a commit to vzotova/nucypher that referenced this pull request Dec 18, 2019

Apply suggestions from code review nucypher#1470

6d28cb5

Co-Authored-By: Derek Pierre <derek.pierre@gmail.com>

KPrasch approved these changes Dec 18, 2019

View reviewed changes

cygnusv reviewed Dec 19, 2019

View reviewed changes

vzotova added a commit to vzotova/nucypher that referenced this pull request Dec 20, 2019

Apply suggestions from code review nucypher#1470

7fd8703

vzotova added a commit to vzotova/nucypher that referenced this pull request Dec 20, 2019

Apply suggestions from code review nucypher#1470

0473812

Co-Authored-By: Derek Pierre <derek.pierre@gmail.com> Co-Authored-By: David Núñez <david@nucypher.com>

vzotova force-pushed the upgradability-test branch from 6d28cb5 to 0473812 Compare December 20, 2019 08:06

cygnusv approved these changes Dec 24, 2019

View reviewed changes

vzotova and others added 14 commits December 24, 2019 19:01

Removed unnecessary * from solidity docs

79f5020

SolidityCompiler: ability to use multiple directories for compilation

b140a03

Versions for upgradeable contracts, deploy contract using version

4971a3d

Test for multi-source solidity compilation

7002734

Upgradability test: download contracts from github, master branch

4ce0a36

Deployers: overrides for constructor parameters

df2fbc2

Deployers: version as input parameter

06d9f39

Versioned contract class, contract version in registry, ability to re…

24b7fc3

…move parameters from constructor in deployers (overrides)

Search using contract version, checking version while deployment

98185a9

Test for multi-version contract deployment

adb11d6

Completed upgradeability test

52bba78

Fixed tests after contract version integration

9def52d

Doc updates for contract versions

d812176

Apply suggestions from code review nucypher#1470

4dcb5b2

Co-Authored-By: Derek Pierre <derek.pierre@gmail.com> Co-Authored-By: David Núñez <david@nucypher.com>

vzotova force-pushed the upgradability-test branch from 0473812 to 4dcb5b2 Compare December 24, 2019 16:02

cygnusv merged commit 11e7fc6 into nucypher:master Dec 26, 2019

vzotova deleted the upgradability-test branch April 28, 2020 13:07

Conversation

vzotova commented Nov 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KPrasch Dec 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vzotova Dec 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derekpierre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cygnusv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vzotova commented Nov 22, 2019 •

edited

Loading

codecov bot commented Nov 22, 2019 •

edited

Loading

KPrasch Dec 11, 2019 •

edited

Loading

vzotova Dec 11, 2019 •

edited

Loading