[TRA-98] Update upper limit of subaccount number constraint #1164
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange>
WalkthroughThe changes involve expanding the maximum number of allowable subaccount IDs in the system from 127 to 128,000, significantly broadening the range of valid subaccount IDs. Test cases were adjusted to align with this change, considering a subaccount ID of 128,001 as invalid due to the new maximum limit. 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 (
|
![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: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- protocol/x/subaccounts/types/subaccount.go (1 hunks)
- protocol/x/subaccounts/types/subaccount_test.go (1 hunks)
Additional comments: 2
protocol/x/subaccounts/types/subaccount.go (1)
- 14-14: The update to
MaxSubaccountIdNumberfrom 127 to 128,000 is clear and aligns with the PR's objectives to accommodate a larger range of subaccount numbers. Ensure that the broader implications of this change on system performance and storage have been considered.protocol/x/subaccounts/types/subaccount_test.go (1)
- 56-56: The update to the test case for an "invalid number" in
TestSubaccountIdValidatecorrectly reflects the new upper limit of subaccount numbers. Ensure that all related test cases are updated to comprehensively test the new range of valid subaccount numbers.
vincentwschau
approved these changes
Mar 11, 2024
Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
protocol/x/subaccounts/types/subaccount.pb.gois excluded by:!**/*.pb.go
Files selected for processing (7)
- proto/dydxprotocol/subaccounts/subaccount.proto (1 hunks)
- protocol/x/clob/types/message_cancel_order_test.go (1 hunks)
- protocol/x/clob/types/message_place_order_test.go (1 hunks)
- protocol/x/sending/types/message_create_transfer_test.go (2 hunks)
- protocol/x/subaccounts/module_test.go (2 hunks)
- protocol/x/subaccounts/types/errors.go (2 hunks)
- protocol/x/subaccounts/types/genesis_test.go (2 hunks)
Additional comments: 11
proto/dydxprotocol/subaccounts/subaccount.proto (1)
- 14-15: The comment update on lines 14-15 clearly communicates the new upper limit for subaccount numbers per owner, aligning with the PR's objective to increase the limit to 128,000. This change enhances the documentation for future reference and is consistent with the PR's goals.
protocol/x/subaccounts/types/errors.go (2)
- 5-8: The addition of imports on lines 5-8 is appropriate and necessary for the subsequent changes in this file. The imported packages are used for error handling and converting integer values to strings, which is crucial for the dynamic error message introduced later.
- 24-28: The modification of the
ErrInvalidSubaccountIdNumbererror message on lines 24-28 to include a dynamic value (MaxSubaccountIdNumber) is a significant improvement. It ensures that the error message remains accurate even if the maximum subaccount ID number changes in the future. This change enhances maintainability and clarity for developers and users.protocol/x/clob/types/message_cancel_order_test.go (1)
- 32-32: The change on line 32, updating the
Numberfield value from9999to999_999in the test case, aligns with the PR's objective to increase the upper limit of subaccount numbers. This adjustment ensures that the test reflects the new constraints and continues to validate the system's behavior correctly.protocol/x/sending/types/message_create_transfer_test.go (2)
- 52-52: The update on line 52, changing the
Numberfield value forSenderfrom9999to999_999, correctly reflects the new upper limit for subaccount numbers. This change ensures the test remains relevant and accurately assesses the system's behavior under the updated constraints.- 66-66: Similarly, the change on line 66 for the
Recipient'sNumberfield value to999_999is consistent with the PR's objectives and the updated subaccount number constraints. It's crucial for maintaining the accuracy and relevance of the test coverage.protocol/x/subaccounts/types/genesis_test.go (2)
- 6-7: The adjustment in the import order on lines 6-7, moving
errorsmod "cosmossdk.io/errors"to a different position, is a minor change that does not impact the functionality. However, maintaining a consistent and logical import order can improve code readability.- 89-95: The modification of the test case on lines 89-95 to check for an
id numbergreater than128_000instead of128is crucial for validating the new upper limit for subaccount numbers. This change ensures that the test accurately reflects the updated constraints and continues to provide meaningful coverage.protocol/x/subaccounts/module_test.go (2)
- 12-14: The addition of imports for
libandtypeson lines 12-14 is necessary for the subsequent changes in this file, particularly for usinglib.IntToStringin the dynamic error message comparison. This ensures the test remains accurate and maintainable.- 138-142: The update to the test case on lines 138-142, using a different subaccount number (
128001) and comparing it dynamically againsttypes.MaxSubaccountIdNumber, is a significant improvement. It ensures the test remains relevant under the new subaccount number constraints and accurately assesses the system's behavior.protocol/x/clob/types/message_place_order_test.go (1)
- 35-35: The change on line 35, updating the
Numbervalue in theSubaccountIdstruct from9999to999_999, aligns with the PR's objective to increase the upper limit of subaccount numbers. This adjustment ensures that the test reflects the new constraints and continues to validate the system's behavior correctly.
Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange>
Comment on lines
+11
to
+12
| * The unique number of this subaccount for the owner. | ||
| * Currently limited to 128*1000 subaccounts per owner. |
There was a problem hiding this comment.
The updated description for the number field in the SubaccountId interface correctly reflects the new limit of 128,000 subaccounts per owner. However, for better readability and to avoid potential confusion, consider explicitly stating the number "128,000" instead of "128*1000".
- Currently limited to 128*1000 subaccounts per owner.
+ Currently limited to 128,000 subaccounts per owner.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.
Suggested change
| * The unique number of this subaccount for the owner. | |
| * Currently limited to 128*1000 subaccounts per owner. | |
| * The unique number of this subaccount for the owner. | |
| * Currently limited to 128,000 subaccounts per owner. |
Comment on lines
+23
to
+24
| * The unique number of this subaccount for the owner. | ||
| * Currently limited to 128*1000 subaccounts per owner. |
There was a problem hiding this comment.
The updated description for the number field in the SubaccountIdSDKType interface correctly reflects the new limit of 128,000 subaccounts per owner. For consistency with the previous comment and to enhance readability, consider explicitly stating the number "128,000" instead of "128*1000".
- Currently limited to 128*1000 subaccounts per owner.
+ Currently limited to 128,000 subaccounts per owner.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.
Suggested change
| * The unique number of this subaccount for the owner. | |
| * Currently limited to 128*1000 subaccounts per owner. | |
| * The unique number of this subaccount for the owner. | |
| * Currently limited to 128,000 subaccounts per owner. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changelist
Increase upper limit of subaccount number to 128,000 to allow for isolated margin subaccounts
Test Plan
Unit tests
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.