[WIP] Standard Solc Compile (Multiversion) #2051

KPrasch · 2020-05-27T10:57:52Z

Based on #1831, #2078; Needs rebase pre-merge.

What this does

Deprecates SolidityCompiler - Rework for a functional approach
Introduces compile package
Updates solidity compilation from the legacy to the standard compile method.
Accommodate Standard JSON input configuration values
Hypothesis testing for devdoc regex full-matches
~~Relock Dependencies to include Hypothesis and include db in .gitignore~~ Moved to Relock dependencies; Include hypothesis. #2077
~~Nonce stability testing (discovered accidentally)~~ Moved to PR [Concept] Nonce stability testing #2074
~~Fixes Duplicate apidoc builds~~ Moved to PR Fixes duplicate apidoc build #2076
~~Fixes Decorator to skip circleCI #1909 - Use a decorator to skip tests on CI~~ Moved to Fixes #1909 - Use a decorator to skip tests on CI #2078

TODO

Multi-version remote management
Arrange Constants
Unskip upgradeabilty test

codecov · 2020-06-06T00:46:24Z

Codecov Report

Merging #2051 into main will decrease coverage by 0.87%.
The diff coverage is 86.04%.

@@            Coverage Diff             @@
##             main    #2051      +/-   ##
==========================================
- Coverage   83.64%   82.76%   -0.88%     
==========================================
  Files         102      107       +5     
  Lines       15066    14733     -333     
==========================================
- Hits        12602    12194     -408     
- Misses       2464     2539      +75

Flag	Coverage Δ
#all	`82.76% <86.04%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
nucypher/blockchain/eth/multisig.py	`61.17% <ø> (ø)`
nucypher/characters/lawful.py	`89.33% <ø> (+1.40%)`	⬆️
nucypher/cli/commands/multisig.py	`35.46% <ø> (ø)`
nucypher/blockchain/eth/sol/compile/aggregation.py	`68.51% <68.51%> (ø)`
nucypher/blockchain/eth/sol/compile/solc.py	`75.00% <75.00%> (ø)`
nucypher/config/constants.py	`94.28% <75.00%> (-5.72%)`	⬇️
nucypher/blockchain/eth/interfaces.py	`72.24% <88.46%> (-4.09%)`	⬇️
nucypher/blockchain/eth/sol/compile/collect.py	`91.30% <91.30%> (ø)`
nucypher/blockchain/eth/sol/compile/compile.py	`100.00% <100.00%> (ø)`
nucypher/blockchain/eth/sol/compile/config.py	`100.00% <100.00%> (ø)`
... 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 85b645c...6b378bc. Read the comment docs.

nucypher/blockchain/eth/interfaces.py

fjarri · 2020-06-08T22:13:24Z

nucypher/blockchain/eth/sol/compile.py


-class SolidityCompiler:
+# TODO: Is there a better way to specify complex dictionaries with variable keys?
+CompiledContracts = TypeVar('CompiledContracts', bound=Dict[str, Dict[str, List[Dict[str, Union[str, List[Dict[str, str]]]]]]])


May be more readable if classes are used instead of anonymous dicts.

Good Idea 👍

vzotova

I like idea to make compilation in functional way, but right now multiversion compilation is not finished which was main idea for upgradeability test

vzotova · 2020-06-09T11:22:23Z

nucypher/blockchain/eth/sol/compile/aggregation.py

+    versioned: bool = version_specifier != DEFAULT_VERSION_STRING
+    if versioned and new_title:
+        existing_title = existing_version['devdoc'].get('title')
+        if existing_title == new_title:  # This is the same contract


Can we use contract name here instead of title? Because contract name we use everywhere through deploy and can control it but title is just a string

Yes, I think so, but it will require passing in contract name from a higher level... unless its included elsewhere in the compiler output.

Ultimately, the reason I am using title here, is because I am only handling the compiled output of a single contract out-of-context from what contract it is.

If it will be hard to get contract name then current approach is ok, but anyway I would prefer to not use title

vzotova · 2020-06-09T11:29:14Z

nucypher/blockchain/eth/sol/compile/aggregation.py

+        return DEFAULT_VERSION_STRING
+
+    # RE Full Match
+    raw_matches = DEVDOC_VERSION_PATTERN.fullmatch(devdoc_details)


Extracting constants to separate file makes such things hard to read, also these constants are mostly internal for one module of compile, so what do you think about simplify this and put them closer to code where they are used?

maybe move DEFAULT_VERSION_STRING here too?

vzotova · 2020-06-09T11:36:47Z

nucypher/blockchain/eth/sol/compile/config.py

+    # It is activated together with the global optimizer setting and can be deactivated here.
+    # Before Solidity 0.6.0 it had to be activated through this switch.  Also see 'yulDetails options'.
+    yul=False
+)


maybe comment out this block? because it's unused and also some of values differ from default values

Which values are different form the defaults? From the docs :

The "enabled" switch above provides two defaults which can be tweaked here.

yul is true by default as far as I know, for example

vzotova · 2020-06-09T11:39:12Z

nucypher/blockchain/eth/sol/compile/config.py

+    f"contracts={NUCYPHER_CONTRACTS_DIR.resolve()}",
+    f"{ZEPPELIN}={ZEPPELIN_DIR.resolve()}",
+    f"{ARAGON}={ARAGON_DIR.resolve()}",
+]


Solidity 0.6.9 introduces --base-path option, if we specify this option as base dir for contracts (SOLIDITY_SOURCE_ROOT) then no need to use remappings option at all and also allowed paths can be removed
But not sure how to put it in configuration

nucypher/blockchain/eth/sol/compile/constants.py

tests/acceptance/blockchain/interfaces/test_handle_multiversion_contracts.py

vzotova · 2020-06-09T12:13:14Z

nucypher/blockchain/eth/sol/compile/compile.py

+    return compiler_output
+
+
+def multiversion_compile(solidity_source_dirs: Tuple[Path, ...], compiler_version_check: bool = True) -> VersionedContractOutputs:


this method can't fully compile multiple versions from multiple sources, because tuple of paths uses same configuration including remappings. For example, we add test file that uses new logic of new main contract, and when we will test upgreadability this method will use same remmapings both for old and new contracts. And as a result - fail when it should not

vzotova · 2020-06-09T12:15:28Z

tests/acceptance/blockchain/interfaces/test_solidity_compiler.py

-        for version, data in contract_data.items():
-            data.pop("ast")
+    # TODO: Remove AST because id in tree node depends on compilation scope <<< Still relevant?
+    interfaces = multiversion_compile(solidity_source_dirs=testerchain.SOURCES)


this does not test initial idea, because for both contract sets used same input tuple and there are no merging between same two equal version of contracts as it was before

vzotova · 2020-06-09T12:17:21Z

tests/contracts/test_contracts_upgradeability.py

@@ -93,17 +92,15 @@ def deploy_earliest_contract(blockchain_interface: BlockchainDeployerInterface,
        pass  # Skip errors related to initialization


-@pytest.mark.slow
+@pytest.mark.skip('GH 403')  # FIXME
 def test_upgradeability(temp_dir_path, token_economics):


All stuff with multi version compilation initially was done to use it here, so it's sort of marker of readiness of this PR, so I think that this can't be skipped in this set of changes

…n class attrs; Accept nonce atability testing with delay.

…_ is None.

…s to use standard compiler output.

…ler input config

vzotova · 2020-08-08T11:19:11Z

nucypher/blockchain/eth/sol/compile/aggregation.py

+    versioned: bool = version_specifier != DEFAULT_VERSION_STRING
+    if versioned and new_title:
+        existing_title = existing_version['devdoc'].get('title')
+        if existing_title == new_title:  # This is the same contract


If it will be hard to get contract name then current approach is ok, but anyway I would prefer to not use title

vzotova · 2020-08-08T11:53:52Z

nucypher/blockchain/eth/sol/compile/aggregation.py

+        return DEFAULT_VERSION_STRING
+
+    # RE Full Match
+    raw_matches = DEVDOC_VERSION_PATTERN.fullmatch(devdoc_details)


maybe move DEFAULT_VERSION_STRING here too?

vzotova · 2020-08-08T11:58:06Z

nucypher/blockchain/eth/sol/compile/solc.py

+
+    # Prepare Solc Command
+    solc_binary_path: str = get_executable(version=compiler_version)
+    SOLC_LOGGER.info(f"Compiling with base path")  # TODO: Add base path


there is log variable few lines upper, and here some constant logger, I guess should be only one

vzotova · 2020-08-08T12:01:04Z

nucypher/blockchain/eth/sol/compile/solc.py

+
+    _allow_paths = f'{SOLIDITY_SOURCE_ROOT},{TEST_SOLIDITY_SOURCE_ROOT}'
+    if allow_paths:
+        _allow_paths += ','.join(str(p) for p in allow_paths)


allow paths will always come from source bundle (or at least should), so how about to remove constants from here?

vzotova · 2020-08-08T12:02:47Z

nucypher/characters/lawful.py

@@ -217,7 +217,7 @@ def create_policy(self, bob: "Bob", label: bytes, **policy_params):
            policy = FederatedPolicy(alice=self, **payload)

        else:
-            # Sample from blockchain via PolicyManager
+            # Sample from nucypher.blockchain.via PolicyManager


looks like autoreplacement mistype

vzotova · 2020-08-08T12:09:26Z

tests/acceptance/blockchain/deployers/test_token_deployer.py

@@ -25,7 +25,7 @@ def test_token_deployer_and_agent(testerchain, deployment_progress, test_registr

    origin = testerchain.etherbase_account

-    # Trying to get token from blockchain before it's been published fails
+    # Trying to get token from nucypher.blockchain.before it's been published fails


vzotova · 2020-08-08T12:15:35Z

tests/metrics/estimate_gas.py

@@ -175,7 +181,7 @@ def estimate_gas(analyzer: AnalyzeGas = None) -> None:
        compiled_contract = testerchain._raw_contract_cache[contract_name]

        version = list(compiled_contract).pop()
-        bin_runtime = compiled_contract[version]['bin-runtime']
+        bin_runtime = compiled_contract[version]['evm']['bytecode']['object']


Are these values fully identical?

vzotova · 2020-08-08T12:28:14Z

tests/utils/blockchain.py

+    SOURCES: Tuple[SourceBundle, ...] = (
+        *BlockchainDeployerInterface.SOURCES,
+        SourceBundle(base_path=TEST_SOLIDITY_SOURCE_ROOT)
+    )


I think this should be one source bundle (same as BlockchainDeployerInterface.SOURCES), and I'm still for variable to hold source bundles :)

vzotova · 2020-08-08T12:31:06Z

tests/acceptance/blockchain/interfaces/test_testerchain.py

-    blockchain_interface.connect()
+    blockchain_interface = BlockchainDeployerInterface(provider_uri='tester://pyevm/2')
+    blockchain_interface.connect(compile_now=False)
+    blockchain_interface._raw_contract_cache = compiled_contracts


This way to test exclude all multiversion compilation handling inside BlockchainDeployerInterface and test becomes less meaningful

KPrasch · 2020-10-29T17:24:07Z

Closing for staleness - to be revisited. Opened #2413

KPrasch requested a review from vzotova May 27, 2020 11:03

KPrasch force-pushed the compile-std branch from cfba7a5 to 19829ae Compare May 27, 2020 11:05

KPrasch added Code Quality 🔧 Pertaining to code quality improvements Configuration Node configuration ETH Contracts labels May 27, 2020

KPrasch force-pushed the compile-std branch from 401fc34 to 683deab Compare May 28, 2020 17:36

KPrasch force-pushed the compile-std branch 5 times, most recently from f4b2e95 to 3c61a2b Compare June 6, 2020 00:27

KPrasch force-pushed the compile-std branch 3 times, most recently from 3bbe51a to fda1f3a Compare June 7, 2020 17:34

KPrasch changed the title ~~[WIP] Standard Solc Compile~~ Standard Solc Compile Jun 8, 2020

KPrasch marked this pull request as ready for review June 8, 2020 18:39

KPrasch requested review from vepkenez, cygnusv, derekpierre, fjarri, jMyles, michwill, mswilkison and tuxxy June 8, 2020 18:40

KPrasch force-pushed the compile-std branch from 81f1a80 to 34bfe67 Compare June 8, 2020 18:48

fjarri reviewed Jun 8, 2020

View reviewed changes

nucypher/blockchain/eth/interfaces.py Show resolved Hide resolved

fjarri reviewed Jun 8, 2020

View reviewed changes

vzotova requested changes Jun 9, 2020

View reviewed changes

KPrasch changed the title ~~Standard Solc Compile~~ [WIP] Standard Solc Compile Jun 9, 2020

KPrasch and others added 18 commits August 7, 2020 09:29

Seperates multiversion compile test into another module.

f5c13a9

hypothesis validation for devdoc regex. Finish test split.

d6385f7

Drafts simple nonce stability testing helper contract.

02a49e7

Breakout compile functionality into a package.

c9b99a8

Arrange tests to respect upstream changes; Conventionalize testerchai…

43c3e49

…n class attrs; Accept nonce atability testing with delay.

Use absoute import paths for nucypher

5d2422d

Moves nonce testing concept to PR nucypher#2074

2c20284

Compiler test module uses new entry points.

d75345d

Ignore solc compile and economics from autodoc.

b776948

Woring around apidoc issues with compile constants when tests.__file_…

df28b86

…_ is None.

Rescues a test utility from fixtures and modifies estimate gas metric…

8b75935

…s to use standard compiler output.

Newsfragment for solc compiler usage updates.

2a22eea

Implements CachedInterface as a NamedTuple.

bb87857

Follow the yellow brick road

8742ed1

All I want for christmas is a working source bundle

3e1fb18

Jamming with Viki and Derek.

f55834f

Use a calculated relative path with imports and source URLs for compi…

2dd8837

…ler input config

Workaround for invalid / reused nonce in tests.

68533cc

KPrasch force-pushed the compile-std branch 3 times, most recently from 754b3e3 to ccf8afb Compare August 7, 2020 18:20

KPrasch requested a review from vzotova August 7, 2020 18:22

Return of import remappings.

3818943

KPrasch force-pushed the compile-std branch from ccf8afb to 3818943 Compare August 7, 2020 19:17

vzotova reviewed Aug 8, 2020

View reviewed changes

KPrasch mentioned this pull request Oct 29, 2020

Use standard JSON compile inputs for solidity #2413

Closed

KPrasch closed this Oct 29, 2020

vzotova mentioned this pull request Nov 15, 2020

Standard Solc Compile (Multiversion) #2439

Merged

KPrasch deleted the compile-std branch October 25, 2023 18:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Standard Solc Compile (Multiversion) #2051

[WIP] Standard Solc Compile (Multiversion) #2051

KPrasch commented May 27, 2020 •

edited

Loading

codecov bot commented Jun 6, 2020 •

edited

Loading

fjarri Jun 8, 2020

KPrasch Jun 9, 2020

vzotova left a comment

vzotova Jun 9, 2020

KPrasch Jun 9, 2020 •

edited

Loading

vzotova Aug 8, 2020

vzotova Jun 9, 2020

vzotova Aug 8, 2020

vzotova Jun 9, 2020

KPrasch Jun 9, 2020 •

edited

Loading

vzotova Jun 10, 2020

vzotova Jun 9, 2020

vzotova Jun 9, 2020

vzotova Jun 9, 2020

vzotova Jun 9, 2020

vzotova Aug 8, 2020

vzotova Aug 8, 2020

vzotova Aug 8, 2020

vzotova Aug 8, 2020

vzotova Aug 8, 2020

vzotova Aug 8, 2020

vzotova Aug 8, 2020

vzotova Aug 8, 2020

vzotova Aug 8, 2020

KPrasch commented Oct 29, 2020

		return compiler_output


		def multiversion_compile(solidity_source_dirs: Tuple[Path, ...], compiler_version_check: bool = True) -> VersionedContractOutputs:

[WIP] Standard Solc Compile (Multiversion) #2051

[WIP] Standard Solc Compile (Multiversion) #2051

Conversation

KPrasch commented May 27, 2020 • edited Loading

What this does

TODO

codecov bot commented Jun 6, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vzotova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KPrasch Jun 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KPrasch Jun 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KPrasch commented Oct 29, 2020

KPrasch commented May 27, 2020 •

edited

Loading

codecov bot commented Jun 6, 2020 •

edited

Loading

KPrasch Jun 9, 2020 •

edited

Loading

KPrasch Jun 9, 2020 •

edited

Loading