Skip to content
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

Index supply by denom #8517

Merged
merged 16 commits into from
Mar 3, 2021
Merged

Index supply by denom #8517

merged 16 commits into from
Mar 3, 2021

Conversation

sahith-narahari
Copy link
Contributor

Description

closes: #7092


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

x/bank/keeper/keeper.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 2, 2021

Codecov Report

Merging #8517 (7c55f94) into master (2864eb6) will increase coverage by 0.02%.
The diff coverage is 75.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8517      +/-   ##
==========================================
+ Coverage   61.40%   61.42%   +0.02%     
==========================================
  Files         674      671       -3     
  Lines       38397    38354      -43     
==========================================
- Hits        23577    23559      -18     
+ Misses      12336    12314      -22     
+ Partials     2484     2481       -3     
Impacted Files Coverage Δ
x/bank/types/codec.go 50.00% <ø> (+8.82%) ⬆️
x/staking/keeper/pool.go 52.17% <0.00%> (ø)
x/bank/types/genesis.go 52.00% <50.00%> (ø)
x/bank/keeper/keeper.go 72.91% <71.05%> (-1.05%) ⬇️
x/bank/keeper/genesis.go 68.42% <100.00%> (ø)
x/bank/keeper/grpc_query.go 41.33% <100.00%> (ø)
x/bank/keeper/invariants.go 65.62% <100.00%> (ø)
x/bank/keeper/querier.go 74.13% <100.00%> (ø)
x/bank/module.go 55.81% <100.00%> (ø)
x/feegrant/simulation/decoder.go 0.00% <0.00%> (-100.00%) ⬇️
... and 3 more

@sahith-narahari sahith-narahari marked this pull request as ready for review March 2, 2021 16:59
@alessio alessio requested review from jgimeno and gsora March 2, 2021 17:38
Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as long as we paginate GetTotalSupply in a follow up

@sahith-narahari
Copy link
Contributor Author

LGTM as long as we paginate GetTotalSupply in a follow up

Does it make sense to have seperate GetPaginatedTotalSupply as GetTotalSupply is being used across multiple places where the pagination is not necessary?

@aaronc
Copy link
Member

aaronc commented Mar 2, 2021

LGTM as long as we paginate GetTotalSupply in a follow up

Does it make sense to have seperate GetPaginatedTotalSupply as GetTotalSupply is being used across multiple places where the pagination is not necessary?

IMHO anywhere GetTotalSupply is being used without pagination should be viewed as a problem. What if there are 10,000 or 1 million denoms in state? That might seem crazy today but at least in our use case we'll be creating LOTS of micro-currencies.

@alessio
Copy link
Contributor

alessio commented Mar 3, 2021

I agree with @aaronc. Please at the very least let's open a follow-up issue.

@alessio
Copy link
Contributor

alessio commented Mar 3, 2021

@sahith-narahari please add a CHANGELOG entry too

@sahith-narahari
Copy link
Contributor Author

LGTM as long as we paginate GetTotalSupply in a follow up

Does it make sense to have seperate GetPaginatedTotalSupply as GetTotalSupply is being used across multiple places where the pagination is not necessary?

IMHO anywhere GetTotalSupply is being used without pagination should be viewed as a problem. What if there are 10,000 or 1 million denoms in state? That might seem crazy today but at least in our use case we'll be creating LOTS of micro-currencies.

Sure, I'll make a followup issue. Should we also extend the same to GetAllBalances which is more widely used across other modules without pagination

@alessio
Copy link
Contributor

alessio commented Mar 3, 2021

LGTM as long as we paginate GetTotalSupply in a follow up

Does it make sense to have seperate GetPaginatedTotalSupply as GetTotalSupply is being used across multiple places where the pagination is not necessary?

IMHO anywhere GetTotalSupply is being used without pagination should be viewed as a problem. What if there are 10,000 or 1 million denoms in state? That might seem crazy today but at least in our use case we'll be creating LOTS of micro-currencies.

Sure, I'll make a followup issue. Should we also extend the same to GetAllBalances which is more widely used across other modules without pagination

Yes, I think that makes sense too.

@alessio alessio added the A:automerge Automatically merge PR once all prerequisites pass. label Mar 3, 2021
@mergify mergify bot merged commit eb8aaf9 into master Mar 3, 2021
@mergify mergify bot deleted the sahith/index-supply branch March 3, 2021 09:58
@amaury1093
Copy link
Contributor

amaury1093 commented Mar 3, 2021

Is a state migration needed for this? If yes, could you add one in x/bank/legacy/042?

@sahith-narahari
Copy link
Contributor Author

Is a state migration needed for this? If yes, could you add one in x/bank/legacy/042?

Sure, I'll do it in the followup PR along with paginating supply.

@aaronc
Copy link
Member

aaronc commented Mar 3, 2021

Is a state migration needed for this? If yes, could you add one in x/bank/legacy/042?

Yes it is. Could you add this to the tracking issue and reopen it @AmauryM ?

@amaury1093
Copy link
Contributor

#7092 (comment)

@amaury1093 amaury1093 mentioned this pull request Mar 3, 2021
2 tasks
@@ -43,19 +43,6 @@ message Output {
[(gogoproto.nullable) = false, (gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.Coins"];
}

// Supply represents a struct that passively keeps track of the total supply
// amounts in the network.
message Supply {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a proto-breaking change. I'm okay to remove it in the v1beta2 package but not here.

I'm adding it back in the migration PR, and adding a deprecated comment to it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can keep it in the proto files even though it's not used for now. Deprecated is the right step.

@@ -22,6 +22,7 @@ var (
)

func TestDecodeStore(t *testing.T) {
t.Skip()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason this test was skipped?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops sorry, this is unintended. Can you remove it in the store migration PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Index supply by denom
6 participants