-
-
Notifications
You must be signed in to change notification settings - Fork 135
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
feat!: change CompileAPI to ecosystem based compilers [APE-1322] #1626
Conversation
656ce9c
to
739d2bd
Compare
8165591
to
5ba6a4d
Compare
Maintainer note: Changed name to include exclamation point to show that it's a breaking change, scheduled for v0.7 |
@@ -153,7 +153,8 @@ def contracts(self) -> Dict[str, ContractType]: | |||
|
|||
@property | |||
def _cache_folder(self) -> Path: | |||
folder = self.contracts_folder.parent / ".build" | |||
current_ecosystem = self.network_manager.network.ecosystem.name | |||
folder = self.contracts_folder.parent / ".build" / current_ecosystem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, so with this change it would have to rebuild for every different ecosystem
On the one hand, this may actually be pretty useful for networks like arbitrum that don't support newer VM rules yet (current suggestion is downgrade VM rules to Paris), but would need a way for the ecosystem to basically declare it's okay with another ecosystem type's VM rules
Also, I believe ethPM-types will have to change to allow compilation artifacts for multiple ecosystems
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is needed for ethpm-types ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the data structure is ContractType.name ->
ContractType`, but doesn't have a way to delineate between different productions of bytecode using different VM rules/compiler config
Should be something more like ContractType.*bytecode
fields should allow multiple bytecode productions, indexed by compiler settings e.g. ContractType.runtimeBytecode[Compiler.name] -> Bytecode
https://github.com/ApeWorX/ethpm-types/blob/main/ethpm_types/contract_type.py#L79
This may take some thinking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but do we really need this? now if we dont use duplicate plugins per ecosystem, lets say for ethereum, if we dont have solidity and zksolc then contract types shouldnt have any problem in my opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other option is back to what I said before, there's a .build/{ecosystem}/
folder that the ecosystem plugin creates mirror copies of the types that actually end up in the published/cached contract manifest, and those mirrors are kept in sync every time you try to do something on that ecosystem. since, at least in this instance, the goal is that you are using the same source code for that ecosystem, it's not really necessary to have the transpiled code tracked alongside the EVM code (or add it as an additional field to ethpm_types.ContractType
, which should accept additional properties not defined in the model)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldnt really understand this. Manifest and abi files will be synced as we have network option and usage now. Can you give more details on this? What should i do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pulling this piece out for 0.7.0.
#1742
241b6ef
to
a78463d
Compare
a78463d
to
c828c4a
Compare
This pull request is considered stale because it has been open 30 days with no activity. Remove stale label, add a comment, or make a new commit, otherwise this PR will be closed in 5 days. |
This PR was closed because it has been inactive for 35 days. |
This pull request is considered stale because it has been open 30 days with no activity. Remove stale label, add a comment, or make a new commit, otherwise this PR will be closed in 5 days. |
@@ -95,7 +95,7 @@ jobs: | |||
pip install .[test] | |||
|
|||
- name: Run Tests | |||
run: pytest -m "not fuzzing" -s --cov=src -n auto --dist loadscope | |||
run: ape test -m "not fuzzing" -s --cov=src --dist loadscope |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert back to pytest
, ape test
is used for running ape projects not the framework or plugins.
@@ -117,4 +117,4 @@ jobs: | |||
pip install .[test] | |||
|
|||
- name: Run Tests | |||
run: pytest -m "fuzzing" --no-cov -s | |||
run: ape test -m "fuzzing" --no-cov -s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here as well, back to pytest
@@ -153,7 +153,8 @@ def contracts(self) -> Dict[str, ContractType]: | |||
|
|||
@property | |||
def _cache_folder(self) -> Path: | |||
folder = self.contracts_folder.parent / ".build" | |||
current_ecosystem = self.network_manager.network.ecosystem.name | |||
folder = self.contracts_folder.parent / ".build" / current_ecosystem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pulling this piece out for 0.7.0.
#1742
closing as it seems we want to go in a different direction (modifying ethpm-types and using a single manifest) |
What I did
Changed CompilerAPI so that we can select which compilers to be used in compilation process, so same extensions can be used as plugin as long as you dont use them at the same time for ecosystem configurations. .build/ folder now generates summary per ecosystem.
fixes: #
How to verify it
Run tests.
Checklist