feat(evmutil): add allow list for evm-convertible sdk denoms #1590
Conversation
pirtleshell
changed the title
updates to the sections describing functionality will be updated once that functionality actually exists... :)
pirtleshell
force-pushed
the
rp-sdk-coins-to-evm
branch
from
610b44c
to
bffc7ec
Compare
There was a problem hiding this comment.
Looks good.
Just the small change of decimal -> decimals might be good. It can be confusing when interfaces don't match exactly, but just a nit.
Do think we should add the check for a value less than 256. Though, I actually don't know what the behavior would be. Will the contract deployment fail, or will solidity do an implicit conversion and overflow?
field now matches erc20 spec
There was a problem hiding this comment.
Decimal changes look great. Like the usage of math.MaxUint8 👍
One more uncertainty here:
We are modifying parameters, which is a consensus breaking change.
What's the behavior of upgrading with default (empty) upgrade handler, is the default set appropriately? Does GetParams work after upgrade?
Do historical GetParams work and handle the missing value gracefully (without panic)?
Can we write unit tests to cover those cases above?
Once we have those tests, should we consider bumping the consensus version in this PR and add a migration?
|
alrighty @nddeluca, without any migrations the behavior is such that the chain will upgrade, but the param indeed never gets set in the store. upgrading from v0.23.0 to this branch with an empty upgrade handler results in a successful upgrade, but any query to evmutil params (current or historical) fails. presumably if the params were actually used the chain itself may panic as well (or at least no txs/queries using them would work) 😄 i am working on the setup of migrations in evmutil and add in a param with consensus version bump to this pr. |
* adds migrator to evmutil's keeper * sets up Migrate1To2 migration * registers migration in module * updates GetParams to properly handle historic block queries
Description
adds
allowed_native_denomsto evmutil params. the param has validation tests and is included in the module's genesis state but is currently unused. the parameter will be used to allow certain sdk denoms to be converted to erc20s in the evm. in addition to the sdk denom, it includes metadata for the erc20 token that will be deployed.Checklist