-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
perf: x/bank create reverse prefix for denom<->address #9611
Conversation
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.
This looks like the right direction. Thanks @alexanderbez ! We would just need to add a migration (cc @AmauryM)
Since this is going into |
Yes it does |
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.
imo there are 3 things missing, which ideally should go in this PR too:
- migrations as you mentioned. Basically it'll be a
x/bank/legacy/v044
new package with aMigrateStore
function (see 043 for example). And also bumping the module's ConsensusVersion to 3. - I believe InitGenesis doesn't call setBalance anymore, so we need to set this index in InitGenesis too
- updating the SPEC
Co-authored-by: Aaron Craelius <[email protected]>
I just had a look at TestSignWithMultiSignersAminoJSON, it's a out of gas error, i solved it by changing the gas limit to 200000 cosmos-sdk/testutil/testdata/tx.go Lines 37 to 38 in a24a329
maybe the staking ones are related? |
Good catch @AmauryM. I've fixed the gas related tests. However, the only tests failing now are are certain x/staking query related ones. |
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. Please fix the tests.
Did some more debugging to no avail. Somehow using the denom prefix store is causing some of these |
@AmauryM any idea on what's causing the tests to fail here? |
I'm having a look at this |
@alexanderbez I fixed the test, it was again due to insufficient gas, take a look at 9a851f9. Before putting automerge on, could you add a state-breaking changelog entry? |
Ohh goodness, how did I not catch this? Curious why this wasn't more obvious. In any case, yes, I'll add a changelog entry. |
x03 | <denom> | <address>
->0
DenomOwners
to use the new index + an additional query for the balancecloses: #9590
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...
!
to the type prefix if API or client breaking changeCHANGELOG.md
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...
!
in the type prefix if API or client breaking change