Fix: deterministically fetch perp info from state #2341
Conversation
|
Warning Rate limit exceeded@ttl33 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 25 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces a new perpetual position variable for SOL in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| // expectations | ||
| expectedNetCollateral *big.Int | ||
| expectedInitialMargin *big.Int | ||
| expectedMaintenanceMargin *big.Int | ||
| expectedErr error |
| subaccountIdsMap := make(map[types.SubaccountId]struct{}) | ||
| perpIdsMap := make(map[uint32]struct{}) |
There was a problem hiding this comment.
nit: Could be named as sets and not maps.
| } | ||
| } | ||
|
|
||
| // Important: Sort the perpIds to ensure determinism! | ||
| perpIdsOrdered := lib.GetSortedKeys[lib.Sortable[uint32]](perpIdsMap) |
There was a problem hiding this comment.
nit/non-blocking: sortedPerpIds to match convention elsewhere in code?
| // SOL positions | ||
| PerpetualPosition_OneSolLong = *testutil.CreateSinglePerpetualPosition( | ||
| 2, | ||
| big.NewInt(10_000_000_000), // 0.1 SOL |
There was a problem hiding this comment.
nit: Change to = 1 SOL to match name of the constant.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
protocol/testutil/constants/positions.go (1)
102-108: LGTM! Consider adding a comment for clarity.The addition of
PerpetualPosition_OneSolLongis appropriate and consistent with other perpetual position constants in the file. It expands the test coverage to include SOL positions, which is valuable.Consider adding a comment to clarify the meaning of the identifier
2:PerpetualPosition_OneSolLong = *testutil.CreateSinglePerpetualPosition( - 2, + 2, // SOL perpetual identifier big.NewInt(10_000_000_000), // 0.1 SOL big.NewInt(0), big.NewInt(0), )This will help maintain clarity as more perpetuals are added in the future.
protocol/x/subaccounts/keeper/subaccount.go (1)
773-774: LGTM! Improved determinism and code clarity.The changes to the
GetAllRelevantPerpetualsfunction effectively address the issue of non-deterministic behavior caused by map iteration. Sorting the perpetual IDs ensures a consistent order of processing, which is crucial for blockchain applications. The renaming of maps (subaccountIdsMapandperpIdsMap) improves code clarity.Consider using a more memory-efficient data structure for
subaccountIdsMapsince you're only checking for existence. You could usemap[types.SubaccountId]struct{}instead ofmap[types.SubaccountId]bool. This doesn't allocate any memory for the value:-subaccountIdsMap := make(map[types.SubaccountId]bool) +subaccountIdsMap := make(map[types.SubaccountId]struct{})And when setting a value:
-subaccountIdsMap[update.SubaccountId] = true +subaccountIdsMap[update.SubaccountId] = struct{}{}Also applies to: 779-779, 782-782, 784-784, 789-789, 793-795, 798-799
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- protocol/testutil/constants/positions.go (1 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (1 hunks)
- protocol/x/subaccounts/keeper/subaccount_test.go (2 hunks)
🔇 Additional comments not posted (2)
protocol/testutil/constants/positions.go (1)
102-108: Clarify the relationship between this change and the PR objectives.The PR objectives mention addressing non-deterministic behavior in map iteration for perpetual IDs. While the addition of a new SOL position constant is valuable for testing, it's not immediately clear how this change relates to the stated objectives.
Could you please clarify:
- How does this new constant contribute to addressing the non-deterministic behavior issue?
- Are there additional changes in other files that use this constant to implement or test the solution?
To help verify the usage of this new constant, you can run the following script:
This will help us understand how the new constant is being used in the context of the PR objectives.
protocol/x/subaccounts/keeper/subaccount_test.go (1)
6110-6110:⚠️ Potential issueSyntax Error: Invalid 'for range' loop syntax
The loop at line 6110 uses
for range 100, which is invalid Go syntax. To iterate 100 times, you should use:- for range 100 { // run 100 times since it's highly unlikely gas usage is deterministic over 100 times if there's non-determinism. + for i := 0; i < 100; i++ { // run 100 times to check for gas usage determinism.Likely invalid or redundant comment.
| // SOL positions | ||
| PerpetualPosition_OneSolLong = *testutil.CreateSinglePerpetualPosition( | ||
| 2, | ||
| big.NewInt(10_000_000_000), // 0.1 SOL | ||
| big.NewInt(0), | ||
| big.NewInt(0), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding more SOL position constants for comprehensive testing.
While the addition of PerpetualPosition_OneSolLong is valuable, consider expanding the SOL-related test constants to match the coverage provided for other cryptocurrencies like BTC and ETH. This could include:
- A short SOL position
- Different sizes of SOL positions (e.g., 1 SOL, 0.01 SOL)
- Positions with non-zero funding values
Here's an example of additional constants you might want to add:
PerpetualPosition_OneSolShort = *testutil.CreateSinglePerpetualPosition(
2,
big.NewInt(-10_000_000_000), // -0.1 SOL
big.NewInt(0),
big.NewInt(0),
)
PerpetualPosition_OneSolLong = *testutil.CreateSinglePerpetualPosition(
2,
big.NewInt(100_000_000_000), // 1 SOL
big.NewInt(0),
big.NewInt(0),
)
PerpetualPosition_OneSolLong_PositiveFunding = *testutil.CreateSinglePerpetualPosition(
2,
big.NewInt(10_000_000_000), // 0.1 SOL
big.NewInt(1000000), // Some positive funding
big.NewInt(0),
)These additional constants would provide a more comprehensive set of test scenarios for SOL-related functionality.
|
@Mergifyio backport release/indexer/v7.x |
✅ Backports have been created
|
(cherry picked from commit cc1b795)
|
@Mergifyio backport release/protocol/v6.x |
✅ Backports have been created
|
(cherry picked from commit cc1b795)
|
@mergify backport release/protocol/v7.x |
✅ Backports have been created
|
(cherry picked from commit cc1b795)
|
@mergify backport release/protocol/v5.2.x |
✅ Backports have been created
|
(cherry picked from commit cc1b795) # Conflicts: # protocol/testutil/constants/positions.go # protocol/x/subaccounts/keeper/subaccount.go # protocol/x/subaccounts/keeper/subaccount_test.go
Changelist
Problem: The ordering a map iteration in go is not deterministic. If the iteration loop completes, the final result is the same. However, if the loop exits in the middle (ie goes over the gas limit -> abort), then the different sets of perp ids have been iterated on; therefore creates a non-deterministic behavior in terms of gas usage.
Fix: deterministically order the perp ids to iterate over by sorting first.
Test Plan
Unit test
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
Bug Fixes
Tests
GetAllRelevantPerpetualsmethod, ensuring reliability under varying conditions.