-
Notifications
You must be signed in to change notification settings - Fork 4
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(erc20): add indexes at genesis state to track active token pairs #68
Conversation
x/erc20/genesis_test.go
Outdated
}, | ||
} | ||
|
||
for _, tc := range testCases { | ||
|
||
// TODO: fix duplicate problem and uncomment this line | ||
//suite.Nil(tc.genesisState.Validate(), "genesis state should be valid") |
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.
no need to uncomment?
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.
@dudong2
I removed it. Thanks.
5b34645
to
acae2d0
Compare
0927856
to
bb1990b
Compare
- multiple token pairs can exists at the same denom - but only one active token pair can exist by index - so index must be handled as genesis state. without it, we cannot track what is the active token pair from multiple token pairs for same denom
problem - one token pair has two indexes - erc20 index - denom index - so, set token pair per indexes and delete keys is wrong - when iterate erc20 indexes, id is already gone. so it will set empty token pair multiple times solution - set token pair once when iterate token pairs
- currently we already have multiple token pairs for same denom which is not allowed for validation genesis - we have not decided yet for handling this.
Regardless of whether it’s OWNER_EXTERNAL or OWNER_MODULE, both indexes are being managed.
lgtm! |
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.
LGTM, apart from the chore suggestion on the comment
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.
lgtm
Co-authored-by: dongsam <[email protected]>
Description
Naming Change
The following maps exist for the purpose of indexing to find
TokenPair
. I believe the new names better reflect their purpose:DenomMap
-->TokenPairDenomIndex
ERC20Map
-->TokenPairERC20AddressIndex
Aligned the following method names with their original intent and actual actions:
GetDenomMap
-->GetTokenPairIdByDenom
GetERC20Map
-->GetTokenPairIdByERC20Addr
Added indexes as state
Problem
denomMap
) and erc20AddressIndex(=erc20Map
).ibc/13B6057538B93225F6EBACCB64574C49B2C1568C5AE6CCFE0A039D7DAC02BF29
ends up with the TokenPair associated with0x2C68D1d6aB986Ff4640b51e1F14C716a076E44C4
, which is incorrect.0xD32eB974468ed767338533842D2D4Cc90B9BAb46
, as it was registered later and is currently active.Solution
Notes
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...