feat: TotalCoin and TotalCoinKeeper implementation #2671
Conversation
implementation of totalCoin --------- Co-authored-by: ThinhNX <[email protected]>
thinhnx-var
requested review from
jaekwon,
deelawn,
thehowl,
mvertes,
moul,
gfanton,
zivkovicmilos and
piux2
as code owners
github-actions
bot
added
📦 🤖 gnovm
thinhnx-var
changed the title
|
@leohhhn |
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
|
I like this approach because we don't have to iterate through all accounts every time we query the |
|
LGTM. |
…ithub.com/VAR-META-Tech/gno into dev-thinhnx/feat_gno_implement_total_coin
hey @ltzmaxwell , the CI is failing because some of my additions are not yet covered by tests. I will probably handle this after #2319 is merged. As for the functionality, I think it's ready for review |
| @@ -128,15 +130,18 @@ func (bank BankKeeper) SubtractCoins(ctx sdk.Context, addr crypto.Address, amt s | |||
| } | |||
There was a problem hiding this comment.
if acc not exist, we can halt here, right?
There was a problem hiding this comment.
that's right, this's an exception that shouldn't happen
gnovm/tests/file.go
Outdated
| } | ||
|
|
||
| func (tb *testBanker) IssueCoin(addr crypto.Bech32Address, denom string, amt int64) { | ||
| coins, _ := tb.coinTable[addr] | ||
| sum := coins.Add(std.Coins{{denom, amt}}) | ||
| tb.coinTable[addr] = sum | ||
| tb.totalCoin[denom] += amt |
There was a problem hiding this comment.
do we need to check overflow/underflow, as what coins.Add/coins.Sub already did.
gnovm/tests/file.go
Outdated
|
|
||
| totalCoin, ok := overflow.Sub64(tb.totalCoin[denom], amt) | ||
| if !ok { | ||
| panic(fmt.Sprintf("totalCoin overflow/underflow for denom %s while removing %d", denom, amt)) |
There was a problem hiding this comment.
Nitpick: here we can already distinguish whether it's overflow or underflow, right?
There was a problem hiding this comment.
There was a problem hiding this comment.
the prev functions i used actually didn't bring panic info
Kouteki
added
the
review/triage-pending
jefft0
removed
the
review/triage-pending
|
Removed the "review team" label because this is already reviewed by core devs. |
|
@jefft0 hi Sir. |
|
This PR is stale because it has been open 3 months with no activity. Remove stale label or comment or this will be closed in 3 months. |
Contributors' checklist...
BREAKING CHANGE: xxxmessage was included in the descriptionImplementation of #2662
TotalCoinKeeper