Implement bep3 evm native conversion logic #1848
Conversation
There was a problem hiding this comment.
Really good test cases covering dust, too small of balance, and insuffient balances on both the sdk and erc20 sides.
Let's just add some defensive testing to ensure all four bep3 assets are covered. In addition to using a map for denom lookup, removing unused code, and making extracted helpers private (or adding unit tests for them).
Looks great otherwise. Refactoring implementation is optional -- just added my thoughts there on readability & complexity.
DracoLi
force-pushed
the
dl/evm-native-conversion-bep3
branch
from
8ef7925
to
a401485
Compare
| ) | ||
|
|
||
| var ( | ||
| bep3Denoms = map[string]bool{ |
There was a problem hiding this comment.
Nice, normally I'd say use a map[string]struct{}, but with 4 items the benefited clarify and simpler logic in isBep3Asset is nice 👍
| return quotient | ||
| } | ||
|
|
||
| // bep3ERC20AmountToCoinMintAndERC20LockAmount converts 18 decimals erc20 bep3 |
| } | ||
|
|
||
| func (suite *Bep3ConversionTestSuite) TestConvertCoinToERC20_Bep3() { | ||
| for _, denom := range bep3Denoms { |
There was a problem hiding this comment.
This actually makes the tests quite a bit easier to read -- easy to see that all denoms are tested and much more resistant to refactorings that could drop a denom, accidentally rename one, break the check some other unique way, etc
| amountToLock := amount.BigInt() | ||
| amountToMint := amount.BigInt() | ||
|
|
||
| if isBep3Asset(pair.Denom) { |
There was a problem hiding this comment.
I see what you mean now, and this is a nice approach showing there is are two paths in the logic: One for bep3 assets, and one for not. Also, makes it clear that the amounts are only modified in the bep3 case, but also keeps the dirty details down a level in a function call.
* Implement bep3 evm native conversion logic * Update changelog * Fix indentation * Add bep3 conversion keeper tests * make DefaultBEP3ConversionDenoms private * refactor bep3 conversion * update bep3 tests to cover all bep3 assets * minor refactor (cherry picked from commit 3afb656)
* Implement bep3 evm native conversion logic * Update changelog * Fix indentation * Add bep3 conversion keeper tests * make DefaultBEP3ConversionDenoms private * refactor bep3 conversion * update bep3 tests to cover all bep3 assets * minor refactor (cherry picked from commit 3afb656) Co-authored-by: Draco <[email protected]>
…s#1858) * Implement bep3 evm native conversion logic * Update changelog * Fix indentation * Add bep3 conversion keeper tests * make DefaultBEP3ConversionDenoms private * refactor bep3 conversion * update bep3 tests to cover all bep3 assets * minor refactor (cherry picked from commit 3afb656) Co-authored-by: Draco <[email protected]>
Description
Replaces #1834
Checklist