[bugfix][protocol] make funding payment event ordering deterministic #921
Conversation
WalkthroughThe update in the 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? TipsChat with CodeRabbit Bot (
|
| @@ -280,7 +280,10 @@ func (k Keeper) UpdateSubaccounts( | |||
| // Emit an event indicating a funding payment was paid / received for each settled funding | |||
| // payment. Note that `fundingPaid` is positive if the subaccount paid funding, | |||
| // and negative if the subaccount received funding. | |||
| for perpetualId, fundingPaid := range fundingPayments { | |||
| // Note the perpetual IDs are sorted first to ensure event emission determinism. | |||
| sortedPerpsWithFunding := lib.GetSortedKeys[lib.Sortable[uint32]](fundingPayments) | |||
There was a problem hiding this comment.
nit: sortedPerpIds as the variable name?
![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: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/x/subaccounts/keeper/subaccount.go (1 hunks)
Additional comments: 2
protocol/x/subaccounts/keeper/subaccount.go (2)
- 283-289: The changes introduced in lines 283-289 ensure that the perpetual IDs are sorted before emitting funding payment events, which is intended to address the non-deterministic behavior previously reported. This is a logical change that should ensure deterministic ordering of events. However, it is important to verify that the sorting function
lib.GetSortedKeysis correctly implemented and that it does not introduce any side effects or performance issues, especially considering that this function is likely to be called frequently.Verification successful
The previous script did not yield any results, which could be due to the specificity of the pattern used in
ast-grep. To ensure we are not missing any matches due to pattern specificity, let's broaden the search to find any usage ofGetSortedKeyswithout specifying the type.
Since the
ast-grepcommand did not produce any results, I will useripgrep(rg) to search for the string "GetSortedKeys" within the codebase to confirm its existence and implementation.
The
ripgrepsearch has confirmed the existence of theGetSortedKeysfunction in multiple files. To verify its implementation, I will generate a script to view the content ofcollections.gowhere the function is defined.
The
GetSortedKeysfunction is defined in thecollections.gofile and its implementation is as follows:// GetSortedKeys returns the keys of the map in sorted order. func GetSortedKeys[R interface { ~[]K sort.Interface }, K comparable, V any](m map[K]V) []K { keys := make([]K, 0, len(m)) for k := range m { keys = append(keys, k) } sort.Sort(R(keys)) return keys }This function takes a map as input and returns a slice of keys sorted according to the sort order defined by the type
R, which must implementsort.Interface. The function appears to be generic and should work correctly for sorting keys of any type that can be ordered, includinguint32as used in the PR.Based on the implementation, there are no obvious side effects or performance issues, assuming that the sorting interface is implemented efficiently for the types used.
* 283-289: Given the nature of the changes, it would be prudent to consider if any specific labels such as `bug` or `refactor` are applicable to this PR, as per the checklist provided in the PR objectives. This helps in categorizing the PR correctly for future reference and may aid in the changelog generation process.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the implementation of lib.GetSortedKeys for uint32 types. ast-grep --lang go --pattern $'lib.GetSortedKeys[lib.Sortable[uint32]]($_)'Length of output: 76
Script:
#!/bin/bash # Broaden the search to find any usage of GetSortedKeys ast-grep --lang go --pattern $'lib.GetSortedKeys($_)'Length of output: 54
Script:
#!/bin/bash # Search for the string "GetSortedKeys" to confirm its existence and implementation rg 'GetSortedKeys'Length of output: 2980
Script:
#!/bin/bash # View the implementation of GetSortedKeys in collections.go cat protocol/lib/collections.goLength of output: 2807
| // Note the perpetual IDs are sorted first to ensure event emission determinism. | ||
| sortedPerpsWithFunding := lib.GetSortedKeys[lib.Sortable[uint32]](fundingPayments) | ||
| for _, perpetualId := range sortedPerpsWithFunding { | ||
| fundingPaid := fundingPayments[perpetualId] | ||
| ctx.EventManager().EmitEvent( | ||
| types.NewCreateSettledFundingEvent( | ||
| *u.SettledSubaccount.Id, |
There was a problem hiding this comment.
While the author of the PR has stated that tests are not necessary because the changes do not affect the state, only the events, it is still recommended to perform tests to ensure that the deterministic ordering of events does not introduce any regressions or unexpected behavior. This is particularly important because the ordering of events can have downstream effects on clients and services that consume these events.
+ // TODO: Add tests to ensure deterministic ordering of events and check for any regressions.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.
| // Note the perpetual IDs are sorted first to ensure event emission determinism. | |
| sortedPerpsWithFunding := lib.GetSortedKeys[lib.Sortable[uint32]](fundingPayments) | |
| for _, perpetualId := range sortedPerpsWithFunding { | |
| fundingPaid := fundingPayments[perpetualId] | |
| ctx.EventManager().EmitEvent( | |
| types.NewCreateSettledFundingEvent( | |
| *u.SettledSubaccount.Id, | |
| // Note the perpetual IDs are sorted first to ensure event emission determinism. | |
| // TODO: Add tests to ensure deterministic ordering of events and check for any regressions. | |
| sortedPerpsWithFunding := lib.GetSortedKeys[lib.Sortable[uint32]](fundingPayments) | |
| for _, perpetualId := range sortedPerpsWithFunding { | |
| fundingPaid := fundingPayments[perpetualId] | |
| ctx.EventManager().EmitEvent( | |
| types.NewCreateSettledFundingEvent( | |
| *u.SettledSubaccount.Id, |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/x/subaccounts/keeper/subaccount.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/subaccounts/keeper/subaccount.go
lucas-dydx
deleted the
lucas-dydx/protocol-deterministic-funding-event-ordering
branch
|
@Mergifyio backport release/protocol/v2.x |
🟠 Waiting for conditions to match
|
…921) (#1166) Co-authored-by: lucas-dydx <[email protected]>
|
https://github.com/Mergifyio backport release/protocol/v3.x |
🟠 Waiting for conditions to match
|
…921) (#1455) Co-authored-by: lucas-dydx <[email protected]>
Changelist
Make the funding payment event ordering deterministic. Should address this Github issue.
Test Plan
Not tested since the change is fairly simple and doesn't affect anything in state (only events).
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.