fix: precision loss in delegation query #25312
Conversation
…nce and add unit test to verify the change
📝 WalkthroughWalkthroughDelegation query rounding behavior changed by using TokensFromSharesRoundUp when converting shares to tokens in delegationToDelegationResponse. A new GRPC test validates the Delegation query returns the expected integer amount without precision loss. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes(empty) ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
x/staking/keeper/grpc_query.go (1)
641-647: Align redelegation query rounding with delegation query
Replace the floor-based token calculation inredelegationsToRedelegationResponseswith a round-up variant to avoid under-reporting and matchdelegationToDelegationResponse.@@ x/staking/keeper/grpc_query.go:646 - val.TokensFromShares(entry.SharesDst).TruncateInt(), + val.TokensFromSharesRoundUp(entry.SharesDst).TruncateInt(),x/staking/keeper/grpc_query_test.go (4)
83-83: Assert parsing success for delegation amount.Check
okto avoid silently using a zero value on malformed input.- amount, ok := math.NewIntFromString(delegationAmount) + amount, ok := math.NewIntFromString(delegationAmount) + require.True(ok)
77-77: Prefer non-panicking decimal parse in tests.Use the non-
Mustvariant with an assertion for clearer failure reporting.- shares := math.LegacyMustNewDecFromStr(validatorDelegatorShares) + shares, err := math.LegacyNewDecFromStr(validatorDelegatorShares) + require.NoError(err)
75-80: Naming nit: avoid reusingsharesfor two different concepts.Using
sharesfor validator’sDelegatorSharesand again for the delegator’s computed shares is confusing. The earlier diff renames the latter todelegationShares; keep that rename to improve clarity.Also applies to: 84-84
65-99: Broaden coverage of the rounding change.Add parallel assertions for:
DelegatorDelegationsandValidatorDelegationsendpoints (they flow throughdelegationsToDelegationResponses) to ensure consistent round-up behavior.- A case where the pre-fix output underflowed by < 1 uatom to prove the fix.
I can draft these tests if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (2)
x/staking/keeper/grpc_query.go(1 hunks)x/staking/keeper/grpc_query_test.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (2)
x/staking/keeper/grpc_query.go (1)
597-598: Correct fix: round up before truncation to eliminate underflow in query output.Switching to
TokensFromSharesRoundUp(...).TruncateInt()resolves the precision loss described in #25263 without touching core math.x/staking/keeper/grpc_query_test.go (1)
74-75: Ensure correct argument type for NewValidator.
testutil.NewValidatortypically expectssdk.ValAddress; passing raw[]bytemay not compile across SDK versions. Cast explicitly for safety.- validator := testutil.NewValidator(s.T(), PKs[0].Address().Bytes(), PKs[0]) + validator := testutil.NewValidator(s.T(), sdk.ValAddress(PKs[0].Address().Bytes()), PKs[0])
|
needs a changelog! |
|
@kingpinXD lint failing |
Description
The pr refactors the
delegationToDelegationResponsefunction to useTokensFromSharesRoundUpinstead ofTokensFromShares.Closes: #25263
Summary by CodeRabbit
Bug Fixes
Tests