wip: fixing dynamic precompiles. Re-writing integration-tests for werc20 #2343
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Important Auto Review SkippedDraft detected. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
| if slices.Contains(existingPrecompiles, address.String()) { | ||
| return nil, fmt.Errorf("precompile already registered: %s", address) | ||
| if !slices.Contains(existingPrecompiles, address.String()) { | ||
| // return nil, fmt.Errorf("precompile already registered: %s", address) |
There was a problem hiding this comment.
remove if not necessary
There was a problem hiding this comment.
how come we don't want the duplicates check anymore? Should this not return an error?
x/evm/keeper/precompiles.go
Outdated
Comment on lines
90
to
92
| } else { | ||
| WEVMOSAddress = common.HexToAddress(erc20precompile.WEVMOSContractTestnet) | ||
| } |
There was a problem hiding this comment.
There is no safe guard currently for a wrong chain ID - do we want to return an error if that's the case?
… rama/dynamic-fixes # Conflicts: # x/erc20/types/interfaces.go # x/evm/keeper/precompiles.go # x/evm/keeper/static_precompiles_test.go # x/evm/types/params_test.go
MalteHerrmann
requested review from
hanchon and
Vvaradinov
and removed request for
a team
|
The suggestion here is to merge this PR now and then remove the whole WERC-20 precompile on a follow-up PR. This way we can revert/get this code back should it ever be necessary, now that the integration tests have been fixed / updated. The failing tests are not part of the WERC-20 integration tests which are the focus of this PR 👀 |
Vvaradinov
approved these changes
Feb 14, 2024
MalteHerrmann
merged commit 4c35162
into
facs95/dynamic-precompiles
24 of 25 checks passed
Vvaradinov
added a commit
that referenced
this pull request
Feb 21, 2024
* add new param * full implementation * load abi correctly * update contract instance * run make format * rename function * add mocks * run make format * make erc20keeper a pointer on app.go * fix typo * fix EnableDynamicPrecompiles * improve error * add comment to app.go * run make format * review comments * remove data folders * run make format * refractor state_transition * fix lint * run make format * update comments * remove unused functions * wip: fixing dynamic precompiles. Re-writing integration-tests for werc20 (#2343) * wip: fixing dynamic precompiles. Re-writing integration-tests for werc20 * run make format * address some linter warnings * remove unnecessary werc20 event constants * remove duplicate import * revert adding args to deposit and withdraw functions * fix some missing imports * WIP test refactors * WIP more test refactors * re-add wevmos precompile to static precompiles * use base denom for wevmos token pair * more WIP * more WIP * run make format * remove unnecessary level of nesting * run gofumpt * fix tests * remove unused utils and move test runner into integration test file * remove mentions of deposit and withdraw events in tx.go * add some todo comments * remove unnecessary print statement * remove unnecessary migration test flows that now live in the v17 upgrades folder * remove unnecessary ordered instruction for integration tests * only use two keys for the integration tests * address linters --------- Co-authored-by: ramacarlucho <ramacarlucho@users.noreply.github.com> Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com> Co-authored-by: MalteHerrmann <malte@evmos.org> Co-authored-by: MalteHerrmann <MalteHerrmann@users.noreply.github.com> * fix regression * imp(evm): Check for dynamic extensions during EVM initialization (#2356) check for active dynamic precompiles as well as static ones in HasCustomPrecompiles * imp(werc20): Remove WERC-20 precompile and register WEVMOS as standard ERC-20 precompile (#2352) * move WEVMOS contract to contracts directory * remove werc20 precompile * add utility to get token pair from grpc handler * adjust integration tests and upgrade logic to account for WEVMOS being registered as an ERC-20 token pair and precompile * run make format * add changelog entry * make addNewTokenPair public * move RegisterEVMExtensions util to v17 migration logic * remove unused methods * run gofumpt * Update app/upgrades/v17/migration.go * address review comments * remove duplicate params checks in tests * update RegisterERC20Extensions upgrade handler * update NewTokenPair function * remove unnecessary param * update mocks * run make format * delete unnecessary keepers from migration logic * fix test * run make format * dont use new function to register aevmos token pair * remove unused keepers in migration logic * remove incorrect test * fix test for p256 --------- Co-authored-by: MalteHerrmann <MalteHerrmann@users.noreply.github.com> Co-authored-by: Freddy Caceres <facs95@gmail.com> Co-authored-by: facs95 <facs95@users.noreply.github.com> Co-authored-by: Ramiro Carlucho <ramirocarlucho@gmail.com> Co-authored-by: ramacarlucho <ramacarlucho@users.noreply.github.com> * add wevmos pair to local node * tests(strv2): Add unit tests for dynamic precompiles (#2366) * add unit tests * run make format * test expected panic * run make format * check errors --------- Co-authored-by: ramacarlucho <ramacarlucho@users.noreply.github.com> * chore(strv2): Rename static precompiles (#2365) * rename static precompiles * rename upgrade * read evmchannels from params * fix old migration * fix lint * not break protobuf * fix tests --------- Co-authored-by: Vladislav Varadinov <vladislav.varadinov@gmail.com> Co-authored-by: Freddy Caceres <facs95@gmail.com> * remove unnecessary function * remove unnecessary import * make enable dynamic precompiles more efficient * run make format * fix old register proto * address pr comments --------- Co-authored-by: facs95 <facs95@users.noreply.github.com> Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com> Co-authored-by: Ramiro Carlucho <ramirocarlucho@gmail.com> Co-authored-by: ramacarlucho <ramacarlucho@users.noreply.github.com> Co-authored-by: MalteHerrmann <malte@evmos.org> Co-authored-by: MalteHerrmann <MalteHerrmann@users.noreply.github.com> Co-authored-by: Vladislav Varadinov <vladislav.varadinov@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
We need to move wevmos contract to be part of the static precompiles. This introduces several changes.
There were several conflicts from the integration tests setup for werc20. Decided to update to the new integration tests setup.
Closes: XAP-113
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
Reviewers Checklist
All items are required.
Please add a note if the item is not applicable
and please add your handle next to the items reviewed
if you only reviewed selected items.
I have...
Unreleasedsection inCHANGELOG.md