[OTE-256] Add upgrade handler to initialize OI during upgrade handler #1302
Conversation
WalkthroughThe upgrade introduces a significant enhancement to the protocol's handling of perpetual contracts by calculating and setting open interest values based on positions. This functionality ensures the accuracy and consistency of open interest across all perpetuals, enhancing the robustness of the system. Additionally, the upgrade includes tests to verify the correct calculation and updating of open interest post-upgrade, alongside improvements for order placement. 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.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
protocol/testing/containertest/preupgrade_genesis.jsonis excluded by!**/*.json
Files selected for processing (2)
- protocol/app/upgrades/v5.0.0/upgrade.go (3 hunks)
- protocol/app/upgrades/v5.0.0/upgrade_container_test.go (5 hunks)
Additional Context Used
Path-based Instructions (2)
protocol/app/upgrades/v5.0.0/upgrade_container_test.go (1)
**/**:
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/app/upgrades/v5.0.0/upgrade.go (1)
**/**:
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.
Additional comments not posted (4)
protocol/app/upgrades/v5.0.0/upgrade_container_test.go (3)
20-22: The introduction of the constantAliceBobBTCQuantumsis clear and follows Go naming conventions. However, it's important to ensure that the value assigned to this constant is consistent with the application's domain requirements and accurately represents the quantum size intended for Alice and Bob's BTC orders.Ensure the value
1_000_000forAliceBobBTCQuantumsaligns with the expected quantum size for BTC orders in the context of the application.
58-105: TheplaceOrdersfunction correctly broadcasts transactions for placing orders for both Alice and Bob. However, there are a few considerations:
- The use of
require.NoErrorensures that any error in broadcasting a transaction will fail the test immediately, which is appropriate for setup functions in tests.- The hardcoded values, such as
ClientId,SubaccountId, andClobPairId, should be verified to ensure they align with the test's expectations and the application's state during the test.- The
GoodTilBlockvalue is set to20, which implies an expectation about the blockchain's block height during these tests. Ensure this assumption is valid in the test environment.Verify that the hardcoded values used in the
MsgPlaceOrdermessages, includingClientId,SubaccountId,ClobPairId, andGoodTilBlock, are appropriate and align with the test's requirements and the application's expected state.
139-165: ThepostUpgradePerpetualOIsfunction checks the open interest for perpetuals post-upgrade. Considerations include:
- The function assumes that the expected open interest for a perpetual with
Id == 0is equal toAliceBobBTCQuantums. This assumption should be explicitly documented or verified to ensure it aligns with the test's expectations and the upgrade logic.- The use of
assert.Equalfto compare expected and actual open interest values is appropriate. However, ensure that the message format strings ("expected: %v, got: %v") provide sufficient context for debugging any failures.Confirm that the assumption regarding the expected open interest for a perpetual with
Id == 0being equal toAliceBobBTCQuantumsis valid and aligns with the upgrade logic and test expectations.protocol/app/upgrades/v5.0.0/upgrade.go (1)
207-256: TheinitializePerpOpenInterestfunction is a critical addition to the upgrade logic, designed to calculate and set open interest for perpetuals based on position sizes. Key considerations include:
The function uses a map (
perpOIMap) to aggregate open interest for each perpetual, which is an efficient approach for accumulating values.The decision to only record positive positions for total open interest (lines 220-223) is logical, assuming that the total negative position size equals the total positive position size. This assumption should be documented or verified.
The panic on line 243 if a perpetual has non-zero open interest before the upgrade is a strong safety check. Ensure that this precondition is communicated to developers or operators performing the upgrade.
Logging successful initialization of open interest (line 251) and cases where perpetuals have zero open interest at the time of upgrade (line 253) provides valuable debugging information.
Confirm that the assumption regarding the equality of total negative and positive position sizes is valid and documented.
Ensure that the precondition of perpetuals having zero open interest before the upgrade is clearly communicated to relevant stakeholders.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/app/upgrades/v5.0.0/upgrade.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/app/upgrades/v5.0.0/upgrade.go
Additional Context Used
|
@Mergifyio backport release/protocol/v5.x |
✅ Backports have been created
|
* Add subaccountNumber to PerpetualPositionResponseObject (#1274) Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange> * [CT-712] send updates for both normal order matches and liquidation (#1280) * Remove volatile market (#1263) * [SKI-21] Bump slinky version to v0.3.1 (#1275) * Bump slinky version * Add cp number method * clean up socks logging (#1285) * [CT-681] fix liquidated side and offsetting side for indexer delevera… (#1284) * [CT-681] fix liquidated side and offsetting side for indexer deleveraging events * fix test * fix test * fix lint * [TRA-105] Add API for parent subaccount perpetual positions (#1282) Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange> * Use sample rate with stream destroyed stats. (#1294) * Revert "[CT-708] Indexer track e2e latency (#1237)" (#1292) This reverts commit 60b94df. * Fix swagger generation makefile command / regen swagger docs (#1299) * pull dydx fork to generate swagger properly * remove the print * remove vault constants (#1293) * Remove custom ping message from socks (#1301) * Add subaccountNumber to the OrderResponseObject (#1296) Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange> * sample more metrics (#1304) * [OTE-256] Add upgrade handler to initialize OI during upgrade handler (#1302) * Add upgrade handler to initialize OI during upgrade handler * nits * Fix lib.ErrorLogWithError: properly pass in args (#1306) * fix broken tests (#1312) * Explicitly close websockets on errors (#1290) * Increase the number of allowed connections to 8000 (#1317) * [TRA-104] Add parentSubaccountNumber API for orders (#1313) Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange> * Improve Slinky logs to prevent unnecessary logs (#1289) * [SKI-26]: Prevent funding index update with no oracle prices from (#1321) halting indexer * Skip equity tier limit check in PlaceShortTermOrder (#1318) * Skip equity tier limit check in PlaceShortTermOrder * remove tests * Add comment * fix lint (#1323) --------- Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange> Co-authored-by: shrenujb <98204323+shrenujb@users.noreply.github.com> Co-authored-by: jayy04 <103467857+jayy04@users.noreply.github.com> Co-authored-by: Eric Warehime <eric.warehime@gmail.com> Co-authored-by: vincentwschau <99756290+vincentwschau@users.noreply.github.com> Co-authored-by: Jonathan Fung <121899091+jonfung-dydx@users.noreply.github.com> Co-authored-by: Tian <tian@dydx.exchange> Co-authored-by: Teddy Ding <teddy@dydx.exchange> Co-authored-by: roy-dydx <133032749+roy-dydx@users.noreply.github.com> Co-authored-by: Christopher-Li <Christopher-Li@users.noreply.github.com>
Changelist
Test Plan
Container test
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.