feat(cosmos): return errors from NewDecCoinsFromCoins (CON-370) - #3997
feat(cosmos): return errors from NewDecCoinsFromCoins (CON-370)#3997wen-coding wants to merge 2 commits into
Conversation
NewDecCoinsFromCoins returns an error when a Coin amount cannot convert to Dec. Update distribution and legacy fee callers, and reject out-of-range amounts in community-pool ValidateBasic paths. Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Well-scoped change: NewDecCoinsFromCoins now returns an error instead of panicking on out-of-range Int→Dec conversion, every caller in the tree is updated, and the community-pool ValidateBasic paths gain a matching guard with new tests. No blockers; two robustness/consistency suggestions and one test-coverage note.
Findings: 0 blocking | 4 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion]
Int.CanConvertToDecis exercised only at values far from the actual boundary (2^200 passes, 2^256-1 fails). The real cutoff isbitLen(i × 10^18) <= 315, so an off-by-one in the comparison would go undetected. Consider adding a pair of cases at exactly the largest convertible Int and one above it. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
sei-cosmos/x/distribution/keeper/allocation.go(AllocateTokens, BeginBlock) andsei-cosmos/x/distribution/keeper/hooks.go(AfterValidatorRemoved, EndBlock) still halt the chain on an out-of-range coin amount — the PR preserves the pre-existing panic semantics rather than introducing them. Thehooks.gosite is notable because the surrounding comment explains that branch exists specifically to avoid an EndBlock halt from a failed send; the conversion panic sitting inside it reintroduces that same class of hazard (practically unreachable, but the asymmetry is worth a note).
| // update outstanding | ||
| outstanding := k.GetValidatorOutstandingRewards(ctx, valAddr).Rewards | ||
| k.SetValidatorOutstandingRewards(ctx, valAddr, types.ValidatorOutstandingRewards{Rewards: outstanding.Sub(sdk.NewDecCoinsFromCoins(commission...))}) | ||
| commissionDec, err := sdk.NewDecCoinsFromCoins(commission...) |
There was a problem hiding this comment.
[suggestion] The conversion check runs after SetValidatorAccumulatedCommission has already written at line 130, so an error here returns with the accumulated commission truncated to remainder while outstanding rewards are untouched and no coins were sent. Every production caller (msg server, precompiles) runs in a revertible context so this can't corrupt committed state today, but it's the opposite ordering from what this same PR does in FundCommunityPool and DistributeFromFeePool, where the conversion is deliberately hoisted above the first mutation. Moving the NewDecCoinsFromCoins(commission...) call up between TruncateDecimal() and SetValidatorAccumulatedCommission makes the validate-then-mutate order uniform and removes the dependence on the caller's revert behavior.
|
|
||
| // CanConvertToDec reports whether i fits in a whole-number Dec (i × 10^Precision). | ||
| func (i Int) CanConvertToDec() bool { | ||
| if i.i == nil { |
There was a problem hiding this comment.
[suggestion] Returning true for a nil Int makes the predicate disagree with the operation it guards: Int{}.ToDec() panics (big.Int.Mul on the nil returned by BigInt()), so a caller that trusts CanConvertToDec() gets a panic anyway. This is unreachable from NewDecCoinsFromCoins (the NewCoins call rejects such coins first), but CanConvertToDec is new exported API on sdk.Int and other callers won't have that guarantee. Returning false for nil — or documenting that the predicate assumes a non-nil Int — would make the contract match ToDec.
Minor, same lines: i.BigInt() already returns a fresh copy, so the wrapping new(big.Int).Set(...) is a redundant allocation.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3997 +/- ##
==========================================
- Coverage 59.08% 57.99% -1.10%
==========================================
Files 2305 2206 -99
Lines 197017 185475 -11542
==========================================
- Hits 116414 107572 -8842
+ Misses 69842 68078 -1764
+ Partials 10761 9825 -936
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Convert commission Coins before writing truncated state, reject nil Ints in CanConvertToDec, and cover the Dec conversion bit-length boundary. Co-authored-by: Cursor <cursoragent@cursor.com>
2 participants
Summary
NewDecCoinsFromCoinsnow returns an error when aCoinamount cannot convert toDec(out-of-range Int→Dec).ValidateBasicpaths reject amounts that cannot convert toDec.Test plan
go test ./sei-cosmos/types/...coveringNewDecCoinsFromCoinsand Int→Dec conversiongo test ./sei-cosmos/x/distribution/...for keeper, msg, and proposal ValidateBasic updatesMade with Cursor