Int64 to math #1319
Int64 to math #1319
Conversation
joe-bowman
requested review from
faddat,
minhngoc274 and
tuantran1702
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe update introduces a significant shift towards using Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 16
Configuration used: CodeRabbit UI
Files ignored due to path filters (8)
docs/swagger.ymlis excluded by:!**/*.ymlx/airdrop/types/messages.pb.gois excluded by:!**/*.pb.gox/claimsmanager/types/claimsmanager.pb.gois excluded by:!**/*.pb.gox/interchainquery/types/messages.pb.gois excluded by:!**/*.pb.gox/interchainstaking/types/interchainstaking.pb.gois excluded by:!**/*.pb.gox/interchainstaking/types/messages.pb.gois excluded by:!**/*.pb.gox/interchainstaking/types/query.pb.gois excluded by:!**/*.pb.gox/participationrewards/types/messages.pb.gois excluded by:!**/*.pb.go
Files selected for processing (32)
- app/upgrades/v1_5.go (2 hunks)
- app/upgrades_test.go (8 hunks)
- proto/quicksilver/claimsmanager/v1/claimsmanager.proto (1 hunks)
- proto/quicksilver/interchainstaking/v1/interchainstaking.proto (2 hunks)
- proto/quicksilver/interchainstaking/v1/query.proto (2 hunks)
- x/claimsmanager/keeper/claims_test.go (4 hunks)
- x/claimsmanager/types/claimsmanager.go (2 hunks)
- x/claimsmanager/types/claimsmanager_test.go (7 hunks)
- x/claimsmanager/types/genesis_test.go (3 hunks)
- x/interchainstaking/keeper/callbacks_test.go (6 hunks)
- x/interchainstaking/keeper/grpc_query.go (1 hunks)
- x/interchainstaking/keeper/grpc_query_test.go (4 hunks)
- x/interchainstaking/keeper/ibc_packet_handlers.go (5 hunks)
- x/interchainstaking/keeper/ibc_packet_handlers_test.go (73 hunks)
- x/interchainstaking/keeper/intent.go (2 hunks)
- x/interchainstaking/keeper/keeper.go (1 hunks)
- x/interchainstaking/keeper/msg_server_test.go (1 hunks)
- x/interchainstaking/keeper/redelegation_record_test.go (11 hunks)
- x/interchainstaking/keeper/redemptions.go (5 hunks)
- x/interchainstaking/keeper/redemptions_test.go (2 hunks)
- x/interchainstaking/keeper/withdrawal_record.go (4 hunks)
- x/interchainstaking/keeper/withdrawal_record_test.go (10 hunks)
- x/interchainstaking/keeper/zones.go (3 hunks)
- x/interchainstaking/keeper/zones_test.go (3 hunks)
- x/participationrewards/keeper/keeper_test.go (1 hunks)
- x/participationrewards/keeper/msg_server_test.go (1 hunks)
- x/participationrewards/keeper/rewards_holdings.go (1 hunks)
- x/participationrewards/keeper/rewards_holdings_test.go (11 hunks)
- x/participationrewards/keeper/submodule.go (2 hunks)
- x/participationrewards/keeper/submodule_liquid.go (4 hunks)
- x/participationrewards/keeper/submodule_osmosis.go (2 hunks)
- x/participationrewards/keeper/submodule_umee.go (4 hunks)
Files not summarized due to errors (1)
- x/interchainstaking/keeper/ibc_packet_handlers_test.go: Error: Message exceeds token limit
Additional comments: 92
x/participationrewards/keeper/submodule.go (1)
- 14-14: Changing the return type of
ValidateClaimfromuint64tomath.Intis a significant improvement for handling large numbers and preventing overflow issues. Ensure that all usages of this function are updated accordingly to handle the new return type.x/claimsmanager/types/claimsmanager.go (2)
- 10-10: Updating the
NewClaimfunction to acceptmath.Intfor theamountparameter enhances the ability to handle large numbers effectively. This change aligns with the PR's objective to mitigate overflow risks.- 27-27: The validation logic in
ValidateBasicnow correctly checks if theamountis not nil and is positive usingmath.Intmethods. This is a robust way to ensure the validity of the amount.proto/quicksilver/claimsmanager/v1/claimsmanager.proto (1)
- 36-41: Replacing the deprecated
_amountfield with a newamountfield of typestringand annotating it forcosmos_proto.scalar,gogoproto.customtype, andgogoproto.nullableis a thoughtful change. It aligns with the goal of handling large numerical values more robustly by leveragingsdkmath.Int. Ensure that all related code and documentation are updated to reflect this change.x/claimsmanager/types/genesis_test.go (1)
- 49-49: Updating the test cases to use
math.Intfor settingAmountvalues aligns with the PR's objective to handle large numerical values more robustly. This change ensures that the tests accurately reflect the new data type usage.Also applies to: 63-63
x/claimsmanager/types/claimsmanager_test.go (1)
- 17-17: Updating the
TestClaim_ValidateBasicfunction to usemath.Intfor theAmountfield is a good practice. It ensures that the tests are aligned with the changes in the data type and accurately test the validation logic with the newmath.Inttype.Also applies to: 34-34, 43-43, 52-52, 61-61, 70-70
x/participationrewards/keeper/submodule_liquid.go (1)
- 23-23: Changing the
ValidateClaimfunction to usemath.Intinstead ofuint64for theamountvariable and initializingamountwithsdk.ZeroInt()is a significant improvement. It aligns with the PR's objective to handle large numerical values more robustly and prevent overflow issues. Ensure that all related logic and tests are updated to accommodate these changes.Also applies to: 29-29, 34-34, 48-48, 59-59, 64-64, 66-66
x/participationrewards/keeper/submodule_osmosis.go (1)
- 75-75: Updating the
ValidateClaimfunction to usemath.Intfor theamountvariable and adjusting the logic to useamount.Add()for accumulation is a good practice. It ensures that the function can handle large numerical values more robustly, aligning with the PR's objectives.Also applies to: 76-76, 82-82, 86-86, 90-90, 97-97, 102-102, 106-106, 111-111, 115-115, 117-117
x/participationrewards/keeper/rewards_holdings.go (1)
- 78-78: Directly using
claim.Amountin theCalcUserHoldingsAllocationsfunction for logging and calculations is a streamlined approach. This change ensures that the logic related to user holdings allocations accurately reflects the transition to usingmath.Intfor handling large numerical values.Also applies to: 85-85, 88-88
x/participationrewards/keeper/submodule_umee.go (1)
- 208-225: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [181-222]
The
ValidateClaimfunction has been updated to usesdkmath.Intinstead ofuint64for theamountvariable and calculations. This change is crucial for handling large numerical values and preventing overflow issues. The use ofsdk.ZeroInt()for initialization and error returns, andamount.Add()for addition operations are correctly implemented according to the new data type requirements. These changes align with the PR's objective to enhance numerical handling within the application.x/claimsmanager/keeper/claims_test.go (3)
- 4-4: The addition of the
cosmossdk.io/mathimport is necessary for usingmath.NewInt()andmath.ZeroInt()in the test cases, aligning with the transition tosdkmath.Int. This change ensures that the tests are updated to reflect the new data type usage in the application.- 16-56: The changes in the test cases to use
math.NewInt()for initializingAmountfields andmath.ZeroInt()for zero values are correctly implemented. These modifications ensure that the tests accurately reflect the transition tosdkmath.Intfor handling numerical values, aligning with the PR's objectives.- 82-95: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [66-92]
The update in
TestKeeper_NewClaim()to change theamounttype fromuint64tomath.Intand usemath.NewInt()for initializing amounts is correctly implemented. This change ensures that the test case accurately reflects the new data type usage in the application, contributing to the overall goal of enhancing numerical handling.proto/quicksilver/interchainstaking/v1/interchainstaking.proto (2)
- 92-97: The transition from the deprecated
_amountfield to the newamountfield with type annotations forsdkmath.Intin theDistributionmessage is correctly implemented. This change aligns with the PR's objective to enhance numerical handling by using a more robust data type for handling large numerical values.- 147-149: Similarly, the transition from the deprecated
_amountfield to the newamountfield with type annotations forsdkmath.Intin theRedelegationRecordmessage is correctly implemented. This change ensures that the proto definitions are updated to reflect the new data type usage, contributing to the overall goal of preventing overflow issues.x/interchainstaking/keeper/intent.go (2)
- 106-106: The simplification of adding claim amounts using
balance.Amount.Add(data.Amount)is correctly implemented. This change enhances the readability and maintainability of the code by removing unnecessary conversions and directly using theAddmethod provided bysdkmath.Int.- 205-205: Similarly, the simplification in
UpdateDelegatorIntentfor adding claim amounts usingclaimAmt.Add(claim.Amount)is correctly implemented. This change maintains consistency in handling numerical values usingsdkmath.Intand contributes to the overall goal of enhancing numerical handling within the application.x/interchainstaking/keeper/redelegation_record_test.go (8)
- 6-6: The addition of the
cosmossdk.io/mathpackage is appropriate for the transition to usingsdkmath.Intfor handling large numerical values. This change aligns with the PR objectives to mitigate overflow risks.- 30-30: Correctly replaced direct integer assignment with
sdkmath.NewInt()for theAmountfield, aligning with the PR objectives to handle large numerical values more reliably.- 78-78: Correctly replaced direct integer assignment with
sdkmath.NewInt()for theAmountfield, aligning with the PR objectives to handle large numerical values more reliably.- 88-88: Correctly replaced direct integer assignment with
sdkmath.NewInt()for theAmountfield, aligning with the PR objectives to handle large numerical values more reliably.- 97-97: Correctly replaced direct integer assignment with
sdkmath.NewInt()for theAmountfield, aligning with the PR objectives to handle large numerical values more reliably.- 137-137: Correctly replaced direct integer assignment with
sdkmath.NewInt()for theAmountfield, aligning with the PR objectives to handle large numerical values more reliably.- 147-147: Correctly replaced direct integer assignment with
sdkmath.NewInt()for theAmountfield, aligning with the PR objectives to handle large numerical values more reliably.- 156-156: Correctly replaced direct integer assignment with
sdkmath.NewInt()for theAmountfield, aligning with the PR objectives to handle large numerical values more reliably.proto/quicksilver/interchainstaking/v1/query.proto (2)
- 100-116: The conversion of integer fields to string fields with specific type annotations in the
Statisticsmessage is correctly implemented. This change aligns with the PR objectives to handle large numerical values more reliably and prevent overflow issues.- 202-206: The conversion of integer fields to string fields with specific type annotations in the
QueryDelegationsResponsemessage is correctly implemented. This change aligns with the PR objectives to handle large numerical values more reliably and prevent overflow issues.x/interchainstaking/keeper/withdrawal_record.go (2)
- 275-275: Correctly using
sdk.NewDecFromIntfor calculatingdistAmountaligns with the transition to usingsdkmath.Intand prevents potential precision loss or conversion issues.- 287-287: Correctly assigning
d.Amountto asdk.Inttype inUpdateWithdrawalRecordsForSlashensures type consistency and aligns with the transition to usingsdkmath.Intfor handling large numerical values.x/interchainstaking/keeper/grpc_query.go (1)
- 192-192: The change to return
Tvl: sumdirectly instead ofTvl: sum.Int64()aligns with the PR's objective to handle large numerical values more reliably usingsdkmath.Int. This is a positive change for preventing overflow issues.Ensure that all parts of the codebase that consume the output of the
Delegationsfunction are updated to handlesdkmath.Intcorrectly.Verification successful
Based on the provided context and the analysis of the shell script output, there are no direct references to the
Delegationsmethod being called outside of its definition and usage within thegrpc_query.gofile itself. The references found are either related to different methods or are part of the protobuf definitions, tests, or documentation. Therefore, the change in return type tosdkmath.Intfor theTvlfield in theQueryDelegationsResponsestruct does not directly affect other parts of the codebase in terms of method calls. It's important to ensure that any code consuming theQueryDelegationsResponsestruct is capable of handling thesdkmath.Inttype for theTvlfield correctly.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of the Delegations function to ensure compatibility with the new return type. rg --type go 'Delegations' | grep -v 'grpc_query.go'Length of output: 35558
x/interchainstaking/keeper/withdrawal_record_test.go (10)
- 31-34: The use of
math.NewInt()for specifying amounts in test distributions is correct and aligns with the PR's objective to handle large numerical values more reliably. This change enhances the test's ability to simulate scenarios with large amounts accurately.- 52-55: The modifications in the expected records to use
math.NewInt()for amounts are consistent with the changes in the initial records setup. This ensures that the tests accurately reflect the handling of large numerical values.- 80-83: Using
math.NewInt()for setting up initial records in the "multi record 5% slashing" test case is appropriate. It ensures that the test cases are prepared to handle large numerical values, aligning with the PR's objectives.- 96-99: The use of
math.NewInt()in the setup of additional withdrawal records for the "multi record 5% slashing" test case is consistent and correct. This change supports the PR's goal of accurately handling large numerical values in tests.- 117-120: The expected records for the "multi record 5% slashing" test case correctly use
math.NewInt()for amounts. This consistency in handling numerical values is crucial for accurately testing the system's behavior under various scenarios.- 132-135: In the expected records for the second withdrawal record in the "multi record 5% slashing" test case, the use of
math.NewInt()for amounts, including the adjusted amount for the slashed validator, is correctly implemented. This ensures the test's accuracy in reflecting the system's behavior.- 159-159: The test case for checking overflow handling uses
math.NewInt(9223372036854775807).Add(math.OneInt())to simulate a value just beyond the maximumint64range. This is a good practice for testing the system's ability to handle extreme values without overflow errors.- 177-177: The expected record for the overflow test case correctly uses
math.NewInt(9223372036854775807).Add(math.OneInt())for the amount. This consistency ensures that the test accurately assesses the system's handling of large numerical values.- 202-202: The "mismatch test" case uses
math.NewInt(100000000)for an amount that intentionally exceeds the total amount to simulate an error scenario. This is an effective way to test the system's error handling capabilities.- 220-220: The expected record for the "mismatch test" case correctly uses
math.NewInt(100000000)for the amount, ensuring that the test accurately simulates the intended error scenario.x/interchainstaking/keeper/redemptions.go (7)
- 136-136: The subtraction operation for adjusting the available amount per validator after considering redelegations is correctly implemented using
Sub(). This ensures accurate tracking of available tokens for unbonding or redelegation.- 140-140: Subtracting the redelegation amount from the total available tokens is correctly implemented. This adjustment is necessary for accurately calculating the total amount of tokens that are unlocked and available for further operations.
- 325-325: The check for zero amount in the withdrawal allocation logic is correctly placed. It ensures that the loop continues to the next withdrawal record if the current one has been fully allocated.
- 334-334: Appending a new distribution for the current validator when the allocation can satisfy the withdrawal amount is correctly implemented. This ensures that the distribution reflects the actual allocation from each validator.
- 353-353: The logic to append a distribution with the entire available allocation from the current validator when it cannot wholly satisfy the withdrawal is correct. This approach ensures that allocations are maximized before moving to the next validator.
- 370-370: The condition to check if there are more validators available for allocation is correctly implemented. This ensures that the allocation process can continue with the next validator if the current one's allocation is exhausted.
- 389-393: The calculation of the sum of amounts in distributions for each withdrawal record is correctly implemented. This ensures that the total allocated amount matches the intended withdrawal amount, maintaining consistency in the allocation process.
x/interchainstaking/keeper/zones_test.go (3)
- 246-246: The transition from int64 to
sdkmath.Intfor theAmountfield intypes.RedelegationRecordis correctly implemented. This change aligns with the PR's objective to handle large numerical values more reliably and prevent overflow issues.- 285-285: The transition from int64 to
sdkmath.Intfor theAmountfield intypes.Distributionwithintypes.WithdrawalRecordis correctly implemented. This change is consistent with the PR's goal to enhance the handling of large numerical values.- 289-289: The transition from int64 to
sdkmath.Intfor theAmountfield in anothertypes.Distributionwithintypes.WithdrawalRecordis correctly implemented. This ensures consistency in handling large numerical values across different parts of the codebase.x/participationrewards/keeper/msg_server_test.go (2)
- 519-520: The transition from int64 to
sdkmath.Intfor theAmountfield in the test case setup is correctly implemented. This change ensures that the test cases align with the PR's objective to handle large numerical values more reliably.- 577-577: In the test case "local_callback_value_valid_denom_overflow", the
Amountfield is correctly set usingsdk.NewInt(1000000000000000000).Mul(sdk.NewInt(1000)), which demonstrates handling very large numbers. This is a good practice for testing the system's ability to handle extreme cases without overflow issues.x/participationrewards/keeper/rewards_holdings_test.go (11)
- 48-48: The transition from int64 to
math.NewInt(500)for setting claim amounts is correctly implemented, ensuring that large numerical values are handled safely to prevent overflow issues.- 65-66: Using
math.NewIntfor claim amounts in the test setup is consistent with the PR's objective to handle large numerical values more reliably. This change correctly applies to both users in the test case.- 91-92: Correctly using
math.NewIntfor setting claim amounts in the test setup. This ensures that the application can handle large numerical values without risk of overflow, aligning with the PR's goals.- 117-118: The use of
math.NewIntfor claim amounts is correctly implemented. This change is crucial for handling large numerical values and preventing overflow issues, aligning with the PR's objectives.- 145-146: Correct implementation of
math.NewIntfor setting claim amounts. This change is essential for the application's ability to handle large numerical values safely, preventing potential overflow issues.- 187-188: The transition to
math.NewIntfor claim amounts is correctly implemented, ensuring the application's ability to handle large numerical values reliably and prevent overflow issues.- 244-245: Using
math.NewIntfor claim amounts in the test setup is correctly implemented. This change is in line with the PR's objective to handle large numerical values more reliably and prevent overflow issues.- 343-344: The transition to
math.NewIntfor setting claim amounts in this test case is correctly implemented. This ensures that the application can handle large numerical values without risk of overflow, aligning with the PR's goals.- 356-357: Correctly using
math.NewIntfor claim amounts in the test setup. This change is crucial for handling large numerical values and preventing overflow issues, aligning with the PR's objectives.- 369-370: The use of
math.NewIntfor setting claim amounts is correctly implemented. This change is essential for the application's ability to handle large numerical values safely, preventing potential overflow issues.- 382-383: Transitioning to
math.NewIntfor claim amounts is correctly implemented, ensuring the application's ability to handle large numerical values reliably and prevent overflow issues.x/interchainstaking/keeper/zones.go (1)
- 440-445: The transition from using (u)int64 to
sdkmath.Intfor handling numerical fields, specifically forout.Depositedandout.Supply, is correctly implemented. Usingsdkmath.ZeroInt()for initialization and.Add()for addition ensures that the application can handle large numerical values more reliably, thereby reducing the risk of overflow errors. This change aligns well with the PR's objectives to enhance the system's ability to manage large numerical values effectively.Also applies to: 454-454
x/participationrewards/keeper/keeper_test.go (1)
- 632-632: The transition to using
math.NewIntFromUint64(amount)for theAmountfield in theaddClaimfunction aligns well with the PR's objective to mitigate overflow risks by usingsdkmath.Int. This is a good application of thesdkmath.Intdata type for handling large numerical values.app/upgrades/v1_5.go (2)
- 8-8: The import of
cosmossdk.io/mathis correctly added to facilitate the use ofsdkmath.Intfor handling large numerical values.- 530-539: The logic for merging distributions using
math.Intis correctly implemented to prevent overflow issues and ensure that distributions for the same validator are merged accurately. However, consider optimizing the retrieval of keys fromdistMapto improve performance, especially if the map grows large.app/upgrades_test.go (6)
- 416-416: The change from using direct integer values to
math.NewInt()for theAmountfield inicstypes.Distributionstruct is consistent with the PR's objective to handle large numerical values more reliably. This should help mitigate the risk of overflow issues.- 420-420: The use of
math.NewInt()here is correct and aligns with the PR's goal. It's important to ensure that all related numerical operations elsewhere in the codebase that interact with these values are also usingsdkmath.Intto maintain consistency and prevent type mismatches.- 424-424: Correct implementation of
math.NewInt()for setting theAmount. This change enhances the application's ability to handle large numbers, aligning with the PR's objectives.- 471-489: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [443-547]
All modifications from direct integer values to
math.NewInt()for theAmountfield across variousicstypes.Distributioninstances are correctly implemented. These changes are crucial for handling large numerical values and preventing overflow issues, which is the primary goal of this PR. It's essential to ensure that the rest of the codebase that interacts with these values is also updated to usesdkmath.Intwhere necessary.
- 562-584: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [565-615]
The continued use of
math.NewInt()for settingAmountvalues inicstypes.Distributioninstances is correctly applied and aligns with the PR's objectives. This consistent application across the test cases ensures that the system can handle large numerical values more reliably.
- 704-704: While the change to
math.NewInt()is correctly applied, it's also important to verify that the logic for merging distributions and calculating sums or other operations involving theseAmountvalues is correctly handlingsdkmath.Int. This includes ensuring that arithmetic operations are performed using methods provided bysdkmath.Intto avoid any potential issues with overflow or underflow.x/interchainstaking/keeper/grpc_query_test.go (7)
- 778-778: The use of
math.NewInt(10000000)correctly replaces direct integer values to handle large numbers more reliably and prevent potential overflow issues. This change aligns with the PR's objective to mitigate overflow risks by transitioning tosdkmath.Int.- 782-782: The use of
math.NewInt(20000000)is consistent with the PR's objective and correctly implemented to handle large numbers, preventing potential overflow issues.- 882-882: Replacing direct integer values with
math.NewInt(10000000)in this context is appropriate and aligns with the PR's goal of enhancing the system's ability to handle large numerical values more reliably.- 886-886: The replacement of direct integer values with
math.NewInt(20000000)here is correctly implemented and supports the PR's objective to mitigate overflow risks by usingsdkmath.Int.- 974-974: Using
math.NewInt(10000000)to replace direct integer values is a suitable approach to handle large numbers more reliably, aligning with the PR's objective to prevent potential overflow issues.- 978-978: The implementation of
math.NewInt(20000000)here is consistent with the PR's goal and correctly applied to enhance the system's ability to handle large numerical values.- 1143-1143: The use of
math.NewInt(10000000)to replace direct integer values in this context is appropriate and supports the PR's objective of transitioning tosdkmath.Intto handle large numbers more reliably.x/interchainstaking/keeper/keeper.go (2)
- 797-803: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [11-11]
The import of
sdkmath "cosmossdk.io/math"is correctly added to support the transition tosdkmath.Int.
- 800-800: The transition to
sdkmath.Intfor theAmountfield in theRedelegationRecordstruct aligns with the PR's objectives to handle large numerical values more reliably.x/interchainstaking/keeper/msg_server_test.go (1)
- 256-256: The change from a literal integer to
math.NewInt(3000000)for setting theAmountfield aligns with the PR's objective to handle large numerical values more reliably. This is a good practice to prevent potential overflow issues.x/interchainstaking/keeper/ibc_packet_handlers.go (1)
- 866-866: In the
HandleFailedUndelegatefunction, the variablerelatedAmountis declared but its type is not explicitly mentioned in the provided code segment. Ensure thatrelatedAmountis of typesdkmath.Intto maintain consistency with the PR's objective of handling large numerical values more robustly.x/interchainstaking/keeper/redemptions_test.go (2)
- 113-117: The transition to using
sdkmath.ZeroInt()andsum.Add()for sum calculation aligns with the PR's objectives. However, ensure that_amountToWithdrawPerWithdrawalalso usessdkmath.Intfor itsAmountfield to maintain type consistency in the comparison withinrequire.Equal().- 137-139: The update to use
sdkmath.NewInt()for theAmountfield inSetRedelegationRecord()is consistent with the PR's objectives and enhances the handling of large numerical values. This change is approved.x/interchainstaking/keeper/callbacks_test.go (6)
- 531-531: The transition to
sdkmath.NewInt(1000)for theAmountfield aligns with the PR's objective to mitigate overflow risks by usingsdkmath.Int. This change ensures that large numerical values are handled more reliably.- 550-550: Using
sdkmath.NewInt(950)for theAmountfield is consistent with the PR's goal of enhancing numerical value handling. This modification supports the application's ability to manage large numbers effectively.- 581-585: The update to use
sdkmath.NewInt()for bothAmountfields in theDistributionstruct is a positive change. It ensures that numerical values are handled in a consistent and overflow-resistant manner.- 604-608: Transitioning to
sdkmath.NewInt()for theAmountfields in this hunk supports the PR's objectives well. It demonstrates a commitment to robust and reliable numerical handling within the application.- 639-643: The use of
sdkmath.NewInt()for theAmountfields here is in line with the PR's aim to prevent overflow issues and enhance the handling of large numerical values. This change is beneficial for the application's integrity.- 662-666: Applying
sdkmath.NewInt()for theAmountfields in this final hunk reinforces the PR's goal of improving numerical value management. This ensures that the application can handle large numbers more effectively.
| Recipient: addressutils.GenerateAddressForTestWithPrefix(zone.AccountPrefix), | ||
| Amount: nil, | ||
| BurnAmount: sdk.NewCoin("uqatom", sdk.NewInt(15000000)), | ||
| Txhash: "d786f7d4c94247625c2882e921a790790eb77a00d0534d5c3154d0a9c5ab68f5", |
There was a problem hiding this comment.
Storing tokens or keys directly in source code is a security risk. Consider moving this to a secure storage solution.
There was a problem hiding this comment.
This is a test transaction sha256 hash, not a key
There was a problem hiding this comment.
@joe-bowman, thank you for the clarification. I understand now that it's a test transaction hash and not a sensitive key, so there's no security concern in this case.
| Recipient: addressutils.GenerateAddressForTestWithPrefix(zone.AccountPrefix), | ||
| Amount: nil, | ||
| BurnAmount: sdk.NewCoin("uqatom", sdk.NewInt(15000000)), | ||
| Txhash: "d786f7d4c94247625c2882e921a790790eb77a00d0534d5c3154d0a9c5ab68f5", |
There was a problem hiding this comment.
Storing tokens or keys directly in source code is a security risk. Consider moving this to a secure storage solution.
There was a problem hiding this comment.
This is a test transaction sha256 hash, not a key
There was a problem hiding this comment.
@joe-bowman, thank you for the clarification. Understood, it's a test transaction hash. No concerns then.
x/interchainstaking/keeper/zones.go
Outdated
| @@ -5,6 +5,7 @@ | |||
| "fmt" | |||
| "math" | |||
|
|
|||
| sdkmath "cosmossdk.io/math" | |||
There was a problem hiding this comment.
The import ordering does not follow the Go conventions. Consider reordering the imports to adhere to the Go community standards and improve code readability. Here's a suggested order:
import (
"errors"
"fmt"
"math"
- sdkmath "cosmossdk.io/math"
"github.com/cosmos/cosmos-sdk/store/prefix"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/bech32"
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
distrtypes "github.com/cosmos/cosmos-sdk/x/distribution/types"
stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
+ sdkmath "cosmossdk.io/math"
icqtypes "github.com/quicksilver-zone/quicksilver/x/interchainquery/types"
"github.com/quicksilver-zone/quicksilver/x/interchainstaking/types"
)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| sdkmath "cosmossdk.io/math" | |
| import ( | |
| "errors" | |
| "fmt" | |
| "math" | |
| "github.com/cosmos/cosmos-sdk/store/prefix" | |
| sdk "github.com/cosmos/cosmos-sdk/types" | |
| "github.com/cosmos/cosmos-sdk/types/bech32" | |
| banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" | |
| distrtypes "github.com/cosmos/cosmos-sdk/x/distribution/types" | |
| stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" | |
| sdkmath "cosmossdk.io/math" | |
| icqtypes "github.com/quicksilver-zone/quicksilver/x/interchainquery/types" | |
| "github.com/quicksilver-zone/quicksilver/x/interchainstaking/types" | |
| ) |
| @@ -4,6 +4,7 @@ | |||
| "encoding/json" | |||
| "fmt" | |||
|
|
|||
| "cosmossdk.io/math" | |||
There was a problem hiding this comment.
The import statement for cosmossdk.io/math is not correctly ordered according to the project's import ordering conventions. Consider reordering imports for better readability and consistency with the rest of the codebase.
- "cosmossdk.io/math"
sdk "github.com/cosmos/cosmos-sdk/types"
+ "cosmossdk.io/math"Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| "cosmossdk.io/math" | |
| sdk "github.com/cosmos/cosmos-sdk/types" | |
| "cosmossdk.io/math" |
| @@ -442,7 +442,7 @@ func (k *Keeper) HandleWithdrawForUser(ctx sdk.Context, zone *types.Zone, msg *b | |||
|
|
|||
| dlist := make(map[int]struct{}) | |||
| for i, dist := range withdrawalRecord.Distribution { | |||
| if msg.Amount[0].Amount.Equal(sdk.NewIntFromUint64(dist.Amount)) { // check valoper here too? | |||
| if msg.Amount[0].Amount.Equal(dist.Amount) { // check valoper here too? | |||
There was a problem hiding this comment.
In the HandleWithdrawForUser function, the comparison msg.Amount[0].Amount.Equal(dist.Amount) assumes that msg.Amount has at least one element. This could lead to a runtime panic if msg.Amount is empty. It's safer to check the length of msg.Amount before accessing its elements.
+ if len(msg.Amount) > 0 && msg.Amount[0].Amount.Equal(dist.Amount) {Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if msg.Amount[0].Amount.Equal(dist.Amount) { // check valoper here too? | |
| if len(msg.Amount) > 0 && msg.Amount[0].Amount.Equal(dist.Amount) { // check valoper here too? |
| @@ -1,12 +1,13 @@ | |||
| package types | |||
|
|
|||
| import ( | |||
| "cosmossdk.io/math" | |||
There was a problem hiding this comment.
The import order does not follow the Go conventions. Consider grouping and ordering imports according to the Go standard library, external libraries, and internal packages.
import (
+ "cosmossdk.io/math"
"github.com/ingenuity-build/multierror"
- "cosmossdk.io/math"
sdk "github.com/cosmos/cosmos-sdk/types"
)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| "cosmossdk.io/math" | |
| import ( | |
| "cosmossdk.io/math" | |
| "github.com/ingenuity-build/multierror" | |
| sdk "github.com/cosmos/cosmos-sdk/types" | |
| ) |
| @@ -3,8 +3,9 @@ | |||
| import ( | |||
| "testing" | |||
|
|
|||
| "cosmossdk.io/math" | |||
There was a problem hiding this comment.
The import order does not follow the Go conventions. Consider grouping and ordering imports according to the Go standard library, external libraries, and internal packages.
import (
"testing"
+ "cosmossdk.io/math"
"github.com/stretchr/testify/require"
- "cosmossdk.io/math"
"github.com/quicksilver-zone/quicksilver/utils/addressutils"
"github.com/quicksilver-zone/quicksilver/x/claimsmanager/types"
)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| "cosmossdk.io/math" | |
| import ( | |
| "testing" | |
| "cosmossdk.io/math" | |
| "github.com/stretchr/testify/require" | |
| "github.com/quicksilver-zone/quicksilver/utils/addressutils" | |
| "github.com/quicksilver-zone/quicksilver/x/claimsmanager/types" | |
| ) |
| @@ -3,8 +3,9 @@ | |||
| import ( | |||
| "testing" | |||
|
|
|||
| "cosmossdk.io/math" | |||
There was a problem hiding this comment.
The import order does not follow the Go conventions. Consider grouping and ordering imports according to the Go standard library, external libraries, and internal packages.
import (
"testing"
+ "cosmossdk.io/math"
"github.com/stretchr/testify/require"
- "cosmossdk.io/math"
"github.com/quicksilver-zone/quicksilver/utils/addressutils"
"github.com/quicksilver-zone/quicksilver/x/claimsmanager/types"
)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| "cosmossdk.io/math" | |
| import ( | |
| "testing" | |
| "cosmossdk.io/math" | |
| "github.com/stretchr/testify/require" | |
| "github.com/quicksilver-zone/quicksilver/utils/addressutils" | |
| "github.com/quicksilver-zone/quicksilver/x/claimsmanager/types" | |
| ) |
| @@ -4,6 +4,7 @@ import ( | |||
| "encoding/json" | |||
| "fmt" | |||
|
|
|||
| "cosmossdk.io/math" | |||
There was a problem hiding this comment.
The import order does not follow the Go conventions. Consider grouping and ordering imports according to the Go standard library, external libraries, and internal packages.
import (
"encoding/json"
"fmt"
+ "cosmossdk.io/math"
sdk "github.com/cosmos/cosmos-sdk/types"
- "cosmossdk.io/math"
"github.com/cosmos/cosmos-sdk/types/bech32"
bankkeeper "github.com/cosmos/cosmos-sdk/x/bank/keeper"
"github.com/quicksilver-zone/quicksilver/utils"
"github.com/quicksilver-zone/quicksilver/x/participationrewards/types"
)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| "cosmossdk.io/math" | |
| import ( | |
| "encoding/json" | |
| "fmt" | |
| "cosmossdk.io/math" | |
| sdk "github.com/cosmos/cosmos-sdk/types" | |
| "github.com/cosmos/cosmos-sdk/types/bech32" | |
| bankkeeper "github.com/cosmos/cosmos-sdk/x/bank/keeper" | |
| "github.com/quicksilver-zone/quicksilver/utils" | |
| "github.com/quicksilver-zone/quicksilver/x/participationrewards/types" | |
| ) |
| @@ -8,6 +8,7 @@ | |||
| "strings" | |||
| "time" | |||
|
|
|||
| "cosmossdk.io/math" | |||
There was a problem hiding this comment.
The import order does not follow the Go conventions. Consider grouping and ordering imports according to the Go standard library, external libraries, and internal packages.
import (
"encoding/json"
"errors"
"fmt"
"strconv"
"strings"
"time"
+ "cosmossdk.io/math"
sdk "github.com/cosmos/cosmos-sdk/types"
- "cosmossdk.io/math"
"github.com/cosmos/cosmos-sdk/types/bech32"
"github.com/cosmos/cosmos-sdk/x/bank/keeper"
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
osmosistypes "github.com/quicksilver-zone/quicksilver/third-party-chains/osmosis-types"
osmolockup "github.com/quicksilver-zone/quicksilver/third-party-chains/osmosis-types/lockup"
"github.com/quicksilver-zone/quicksilver/x/participationrewards/types"
)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| "cosmossdk.io/math" | |
| import ( | |
| "encoding/json" | |
| "errors" | |
| "fmt" | |
| "strconv" | |
| "strings" | |
| "time" | |
| "cosmossdk.io/math" | |
| sdk "github.com/cosmos/cosmos-sdk/types" | |
| "github.com/cosmos/cosmos-sdk/types/bech32" | |
| "github.com/cosmos/cosmos-sdk/x/bank/keeper" | |
| banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" | |
| osmosistypes "github.com/quicksilver-zone/quicksilver/third-party-chains/osmosis-types" | |
| osmolockup "github.com/quicksilver-zone/quicksilver/third-party-chains/osmosis-types/lockup" | |
| "github.com/quicksilver-zone/quicksilver/x/participationrewards/types" | |
| ) |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1319 +/- ##
==========================================
+ Coverage 63.33% 63.35% +0.01%
==========================================
Files 171 171
Lines 11314 11313 -1
==========================================
+ Hits 7166 7167 +1
+ Misses 3421 3420 -1
+ Partials 727 726 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (12)
- app/upgrades/v1_5.go (2 hunks)
- x/claimsmanager/keeper/claims_test.go (4 hunks)
- x/claimsmanager/types/claimsmanager.go (2 hunks)
- x/claimsmanager/types/claimsmanager_test.go (7 hunks)
- x/claimsmanager/types/genesis_test.go (3 hunks)
- x/interchainstaking/keeper/redelegation_record_test.go (11 hunks)
- x/interchainstaking/keeper/zones.go (3 hunks)
- x/interchainstaking/keeper/zones_test.go (3 hunks)
- x/participationrewards/keeper/submodule.go (2 hunks)
- x/participationrewards/keeper/submodule_liquid.go (4 hunks)
- x/participationrewards/keeper/submodule_osmosis.go (2 hunks)
- x/participationrewards/keeper/submodule_umee.go (4 hunks)
Files skipped from review as they are similar to previous changes (12)
- app/upgrades/v1_5.go
- x/claimsmanager/keeper/claims_test.go
- x/claimsmanager/types/claimsmanager.go
- x/claimsmanager/types/claimsmanager_test.go
- x/claimsmanager/types/genesis_test.go
- x/interchainstaking/keeper/redelegation_record_test.go
- x/interchainstaking/keeper/zones.go
- x/interchainstaking/keeper/zones_test.go
- x/participationrewards/keeper/submodule.go
- x/participationrewards/keeper/submodule_liquid.go
- x/participationrewards/keeper/submodule_osmosis.go
- x/participationrewards/keeper/submodule_umee.go
| func isNumericString(in string) bool { | ||
| _, err := strconv.Atoi(in) | ||
| return err == nil | ||
| } |
There was a problem hiding this comment.
In lieu of this change (using math.Int instead of uint64), let's add a comment here about why this code is okay: Given that if uqck is the smallest amount ever, maxInt64=((1<<63)-1)/1_000_000 = 9_223_372_036_854.775 (9.223 Trillion) but the highest supply of Quicksilver tokens in general supply is 400_000_000 (400 Million)
I highly recommend instead using strconv.ParseInt(in, 10, 64) and not strconv.Atoi because it fits within the value of bits so if on a 32-bit computer, it'll have a different range than on a 64-bit computer but strconv.ParseInt(..., 10, 64) will always check the range of 64-bits
// It is okay to use strconv.ParseInt to test if a value is numeric because
// The total supply of QCK is 400_000_000 (400 million) QCK aka 400_000_000_000_000 UQCK
// and to parse numeric values, say in the smallest unit of uqck
// MaxInt64: (1<<63)-1 = 9_223_372_036_854_775_807 UQCK = 9_223_372_036_854.775 (9.223 Trillion) QCKThere was a problem hiding this comment.
Thanks for catching this. The usage here is to validate the monotonically increasing value at the end of an lsm share. This is stored as a uint64 in the SDK logic (https://github.com/cosmos/cosmos-sdk/blob/0af2f4da004cbea6414a8bad56e8bdcd45badf1e/x/staking/keeper/tokenize_share_record.go#L14)
In reality it will never come close to 16 bits let alone 32, but happy to parseInt here.
Adds a comment for future selves, about the choice of strconv.ParseInt inside isNumericString and showing that its limits won't be exceeded. Updates PR quicksilver-zone#1319
) Adds a comment for future selves, about the choice of strconv.ParseInt inside isNumericString and showing that its limits won't be exceeded. Updates PR #1319
1. Summary
Fixes potential overflow when using 1e18 base tokens
Migrate a number of (u)int64 fields to sdkmath.Int
2.Type of change
3. Implementation details
Use sdkmath.Int in place of (u)int64 fields
Summary by CodeRabbit