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

Improve set supply #8950

Merged
merged 27 commits into from
Mar 25, 2021
Merged

Improve set supply #8950

merged 27 commits into from
Mar 25, 2021

Conversation

jgimeno
Copy link
Contributor

@jgimeno jgimeno commented Mar 22, 2021

Description

closes: #8931


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

@jgimeno jgimeno changed the title temp commit Improve set supply Mar 22, 2021
@codecov
Copy link

codecov bot commented Mar 22, 2021

Codecov Report

Merging #8950 (7b5645f) into master (8ee9da7) will increase coverage by 0.01%.
The diff coverage is 85.71%.

❗ Current head 7b5645f differs from pull request most recent head 58f2b4e. Consider uploading reports for the commit 58f2b4e to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8950      +/-   ##
==========================================
+ Coverage   58.88%   58.90%   +0.01%     
==========================================
  Files         571      571              
  Lines       32120    32115       -5     
==========================================
+ Hits        18914    18917       +3     
+ Misses      10984    10977       -7     
+ Partials     2222     2221       -1     
Impacted Files Coverage Δ
x/bank/keeper/keeper.go 76.71% <85.18%> (+3.52%) ⬆️
x/bank/keeper/genesis.go 78.94% <100.00%> (ø)

@jgimeno jgimeno marked this pull request as ready for review March 22, 2021 20:23
Copy link
Contributor

@fdymylja fdymylja left a comment

Choose a reason for hiding this comment

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

LGTM! maybe a changelog entry?


bz := k.cdc.MustMarshalBinaryBare(&coin)
supplyStore.Set([]byte(coin.GetDenom()), bz)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not keeping a loop here?

  1. in all places where we use this function we have the following pattern: https://github.com/cosmos/cosmos-sdk/pull/8950/files#diff-9994bff9a7f71878caa036e9c924ab8b06e31490a17874fab72648ed3404d220R346
  2. if we move the loop here, we don't need to recreate and store and do additional allocations. @odeke-em was shaving recently such places.

Copy link
Member

Choose a reason for hiding this comment

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

An argument could be made for doing all of this logic inline inside MintCoins and BurnCoins

Copy link
Member

Choose a reason for hiding this comment

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

@jgimeno I wonder if there's value in making this setSupply(sdk.KVStore, sdk.Coin) to avoid the allocations from ctx.KVStore as @robert-zaremba is suggesting. Wdyt?

supply = supply.Sub(amount)
newSupply = append(newSupply, supply)
}
k.setSupply(ctx, newSupply)
Copy link
Member

Choose a reason for hiding this comment

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

I don't see a need to bundle this into Coins at all. Instead I think we should just be setting supply by denom in the above for loop. The whole Coins abstraction is just unnecessary here IMHO.

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 for inlining. The function is used only 2 times, so not a big trade off.

x/bank/keeper/keeper.go Outdated Show resolved Hide resolved
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.

Would be good to get this supply store stuff right once and for all. So blocking until we have a clear path on these issues.

Mainly:

  • it's maybe unnecessary to have a setSupply method taking sdk.Coins. Can this stuff be dealt with:
    • inline in MintCoins or BurnCoins, or
    • we have setSupply(sdk.Context, sdk.Coin) for a single coin?
  • we don't need Coin serialized in the value of supply keys, just the amount

What do you think @jgimeno ?

supplyStore := prefix.NewStore(store, types.SupplyKey)

for i := range coins {
bz := k.cdc.MustMarshalBinaryBare(&coins[i])
Copy link
Member

Choose a reason for hiding this comment

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

Also noticing that this doesn't seem quite right. There's no need to serialize the whole Coin. We just need the amount serialized in the value. The denom is already serialized in the key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the first approach I took, setSupply to get one coin, I can revert it. I agree on the coin serialization. Thanks!

@alessio alessio added the A:automerge Automatically merge PR once all prerequisites pass. label Mar 24, 2021
@orijbot
Copy link

orijbot commented Mar 24, 2021

@orijbot
Copy link

orijbot commented Mar 24, 2021

@orijbot
Copy link

orijbot commented Mar 25, 2021

@orijbot
Copy link

orijbot commented Mar 25, 2021

@mergify mergify bot merged commit 7ac436d into master Mar 25, 2021
@mergify mergify bot deleted the jonathan/supply-improvement branch March 25, 2021 10:03
@orijbot
Copy link

orijbot commented Mar 25, 2021

@orijbot
Copy link

orijbot commented Mar 25, 2021

@cyberbono3 cyberbono3 self-requested a review March 31, 2021 01:40
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.

x/bank setSupply is inefficient
9 participants