FNS subaccount implementation #2059

dydxwill · 2024-08-07T20:38:40Z

Changelist

Move protos around to not have circular dependency between subaccounts/clob modules
Get snapshot of subaccount
Stream subaccount updates

Test Plan

Tested in dev env

Author/Reviewer Checklist

If this PR has changes that result in a different app state given the same prior state and transaction list, manually add the state-breaking label.
If the PR has breaking postgres changes to the indexer add the indexer-postgres-breaking label.
If this PR isn't state-breaking but has changes that modify behavior in PrepareProposal or ProcessProposal, manually add the label proposal-breaking.
If this PR is one of many that implement a specific feature, manually label them all feature:[feature-name].
If you wish to for mergify-bot to automatically create a PR to backport your change to a release branch, manually add the label backport/[branch-name].
Manually add any of the following labels: refactor, chore, bug.

Summary by CodeRabbit

New Features
- Introduced new streaming functionality for subaccount updates, including new data structures and methods to handle real-time updates.
- Enhanced subscription management for specific subaccounts, improving how updates are processed and delivered.
Bug Fixes
- Adjusted export declarations to ensure proper integration of new modules and their functionalities.
Documentation
- Added new constants for tracking gRPC metrics specific to subaccount updates and snapshots.

into lw/sa2

jonfung-dydx · 2024-08-09T21:13:14Z

protocol/streaming/full_node_streaming_manager.go

+func (sm *FullNodeStreamingManagerImpl) AddSubaccountUpdatesToCache(
+	updates []clobtypes.StreamUpdate,
+	subaccountIds []*satypes.SubaccountId,
+	numUpdatesToAdd uint32,


don't need this field, can just use len(updates)

jonfung-dydx · 2024-08-09T21:21:19Z

protocol/streaming/full_node_streaming_manager.go

-// AddUpdatesToCache adds a series of updates to the full node streaming cache.
+// SendSubaccountUpdates groups subaccount updates by their subaccount ids and
+// sends messages to the subscribers.
+func (sm *FullNodeStreamingManagerImpl) SendSubaccountUpdates(


We could just do SendSubaccountUpdate and not take in a series of subaccount updates. It doesn't quite make sense to have a couple subaccount updates in a row for the same subaccount id since we would only take the last one.

Going to add this as a TODO for now. The current code replicates what's done by Indexer & the client code should cover the aggregation. Created https://linear.app/dydx/issue/CT-1117/aggregate-subaccount-update-per-subaccount-id to track

jonfung-dydx · 2024-08-09T21:21:49Z

protocol/streaming/full_node_streaming_manager.go

+	// Group subaccount updates by subaccount id.
+	streamUpdates := make([]clobtypes.StreamUpdate, 0)
+	subaccountIds := make([]*satypes.SubaccountId, 0)
+	for _, subaccountUpdate := range subaccountUpdates {


We could eliminate the grouping logic by making it take in one subaccount update, same as above comment

StreamUpdate is 1:1 with SubaccountUpdate so we will still need to send multiple StreamUpdates. But yes, if we implement above (added a todo for now), we will only have 1 SubaccountUpdate per subaccountid.

jonfung-dydx · 2024-08-09T21:23:56Z

protocol/streaming/full_node_streaming_manager.go

+	}
+
+	// Remove all subscriptions and wipe the buffer if buffer overflows.
+	if len(sm.streamUpdateCache) > int(sm.maxUpdatesInCache) {


factor this common code out?

jonfung-dydx · 2024-08-09T21:25:39Z

protocol/streaming/full_node_streaming_manager.go

+	defer metrics.ModuleMeasureSince(
+		metrics.FullNodeGrpc,
+		metrics.GrpcSendSubaccountUpdatesLatency,
+		time.Now(),


Can you update the datadog dashboards with subaccount metrics? would be good to know what % of the messages in our update cache are subaccounts, etc

created https://linear.app/dydx/issue/CT-1116/update-datadog-dashboards-for-subaccount-fns to track

jonfung-dydx · 2024-08-09T21:27:47Z

protocol/streaming/full_node_streaming_manager.go

@@ -544,6 +718,14 @@ func (sm *FullNodeStreamingManagerImpl) InitializeNewStreams(
 				}

 				sm.SendSnapshot(allUpdates, subscriptionId, blockHeight, execMode)


Since we have one stream, can we combine everything in just one snapshot function? Users might be confused they get snapshot messages in two chunks, even though they are for subaccounts and orderbooks.

jonfung-dydx · 2024-08-09T21:31:38Z

protocol/x/clob/keeper/keeper.go

@@ -262,6 +263,33 @@ func (k Keeper) InitializeNewStreams(ctx sdk.Context) {
 				clobPairId,
 			)
 		},
+		func(subaccountId satypes.SubaccountId) *satypes.StreamSubaccountUpdate {
+			subaccount := k.subaccountsKeeper.GetSubaccount(


I think we might need to use GetSettledSubaccount to account for funding rates. Depends on what research wants us to emit. Whatever we do, should make sure all our subaccount emissions are settled or non settled.

@jayy04 any thoughts?

This is the subaccount snapshot code. We are accounting for funding rates during the updates.

jonfung-dydx · 2024-08-09T21:34:01Z

protocol/x/subaccounts/keeper/subaccount.go

@@ -369,6 +411,17 @@ func (k Keeper) UpdateSubaccounts(
 			),
 		)

+		// if GRPC streaming is on, emit a generated subaccount update to stream.
+		if streamingManager := k.GetFullNodeStreamingManager(); streamingManager.Enabled() {


Nit for later: we might want to early return if fns doesn't have a current subscription to this subaccount id. This is definitely a hot path in our code.

I think we can't have that outside the FNS manager due to accessing this without the lock. Think we need to define:

def streamingManager.CheckIfSubaccountRegistered :
lock.acquire()
defer lock.release
// check subaccount to subscirption id mapping

Oh, i see this function GetSubaccountIdToSubscriptionIdMapping acquires the lock and returns the map -- I don't think we should expose the pointer to the map in case it is modified somewhere without the lock.

done, created TracksSubaccountId function instead.

coderabbitai

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 1fba8ca and 53bb1be.

Files selected for processing (4)

protocol/streaming/full_node_streaming_manager.go (17 hunks)
protocol/streaming/noop_streaming_manager.go (3 hunks)
protocol/streaming/types/interface.go (4 hunks)
protocol/x/subaccounts/keeper/subaccount.go (4 hunks)

Files skipped from review as they are similar to previous changes (3)

protocol/streaming/noop_streaming_manager.go
protocol/streaming/types/interface.go
protocol/x/subaccounts/keeper/subaccount.go

Additional comments not posted (10)

protocol/streaming/full_node_streaming_manager.go (10)
42-43: Addition of subaccountIdToSubscriptionIdMapping is appropriate.

This field is necessary for managing subaccount-specific subscriptions effectively.

59-60: Addition of subaccountIds in OrderbookSubscription is appropriate.

This field supports the inclusion of subaccount IDs in subscription management.

86-86: Initialization of subaccountIdToSubscriptionIdMapping is appropriate.

This ensures the map is ready for managing subaccount subscriptions.

168-178: Update logic for subaccountIdToSubscriptionIdMapping is consistent.

The logic mirrors that of clobPairIdToSubscriptionIdMapping, ensuring uniform subscription management.

256-269: Removal logic for subaccountIdToSubscriptionIdMapping is consistent.

This consistency helps maintain the integrity of subscription management.

303-319: getSubaccountStreamUpdates method is well-implemented.

The method correctly transforms subaccount updates into stream updates.

505-532: SendSubaccountUpdates method is well-implemented.

The method effectively groups and sends subaccount updates. Note the existing TODO for aggregation.

563-584: AddSubaccountUpdatesToCache method is consistent with existing caching logic.

The method follows established patterns for adding updates to the cache.

Line range hint 664-691: InitializeNewStreams method appropriately includes subaccount snapshots.

The method effectively integrates subaccount snapshot logic with existing orderbook snapshot functionality.

146-153: Handling of subaccount IDs in Subscribe is consistent.

The conversion from pointers to values and the mapping logic align with existing patterns.

Ensure that subaccount ID handling is consistent throughout the codebase.

Verification successful

Handling of subaccount IDs is consistent across the codebase.

The conversion from pointers to values and the mapping logic in the Subscribe method align with existing patterns throughout the codebase. No inconsistencies were found.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify consistent handling of subaccount IDs across the codebase.

# Test: Search for subaccount ID handling patterns. Expect: Consistent usage.
rg --type go -A 5 $'subaccountId'
Length of output: 203726

jonfung-dydx · 2024-08-12T17:23:33Z

protocol/streaming/full_node_streaming_manager.go

+
+	var streamUpdates []clobtypes.StreamUpdate
+	streamUpdates = append(streamUpdates, sm.getOrderbookStreamUpdate(offchainUpdates, blockHeight, execMode)...)
+	streamUpdates = append(streamUpdates, sm.getSubaccountStreamUpdates(saUpdates, blockHeight, execMode)...)


meganit: getSubaccountStreamUpdate seems like some getter function for the full node streaming manager whereas it is a struct wrapping function, can we rename that to something else or just inline define the struct definitions in this function?

renamed to toSubaccountStreamUpdates and toOrderbookStreamUpdate

jonfung-dydx · 2024-08-12T17:25:32Z

protocol/streaming/full_node_streaming_manager.go

@@ -456,7 +597,6 @@ func (sm *FullNodeStreamingManagerImpl) AddUpdatesToCache(
 		sm.streamUpdateCache = nil
 		sm.streamUpdateSubscriptionCache = nil
 	}
-	sm.EmitMetrics()


Is this intended?

nvm, i see it was pulled out

jonfung-dydx · 2024-08-12T17:27:11Z

protocol/streaming/full_node_streaming_manager.go

@@ -410,7 +485,7 @@ func (sm *FullNodeStreamingManagerImpl) SendTakerOrderStatus(
 		clobPairId = takerOrder.OrderId.ClobPairId
 	}

-	sm.AddUpdatesToCache(
+	sm.AddOrderbookUpdatesToCache(


Nit: rename function since this is used for Taker Orders as well

renamed to AddOrderUpdatesToCache

jonfung-dydx · 2024-08-12T17:29:34Z

protocol/streaming/full_node_streaming_manager.go

+// FlushCacheIfFull removes all subscriptions and wipes the buffer if buffer overflows.
+// Note this method requires the lock and assumes that the lock has already been
+// acquired by the caller.
+func (sm *FullNodeStreamingManagerImpl) FlushCacheIfFull() {


Nit: can we rename this? FlushCacheIfFull -> RemoveSubscriptionsAndClearBufferIfFull. Want to make sure it's explicit it's not sending out updates since all our other update emission functions are prefixed by Flush

…wl/sa2

coderabbitai

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 53bb1be and 320b3b0.

Files selected for processing (4)

protocol/streaming/full_node_streaming_manager.go (17 hunks)
protocol/streaming/noop_streaming_manager.go (3 hunks)
protocol/streaming/types/interface.go (4 hunks)
protocol/x/subaccounts/keeper/subaccount.go (4 hunks)

Files skipped from review as they are similar to previous changes (3)

protocol/streaming/full_node_streaming_manager.go
protocol/streaming/types/interface.go
protocol/x/subaccounts/keeper/subaccount.go

Additional comments not posted (5)

protocol/streaming/noop_streaming_manager.go (5)

Line range hint 23-28:
LGTM! The function signature update is appropriate.

The addition of the SubaccountId parameter aligns with the intended subaccount streaming functionality.

54-59: LGTM! The new method aligns with the class's purpose.

The SendSubaccountUpdates method is correctly defined with appropriate parameters for handling subaccount updates.

61-63: LGTM! The function is consistent with the class's purpose.

The TracksSubaccountId method correctly reflects the no-op nature by returning false.

64-69: LGTM! The function signature update is appropriate.

The addition of the getSubaccountSnapshot parameter aligns with the intended subaccount streaming functionality.

Line range hint 71-73: No changes detected in the Stop function.

The function remains unchanged and consistent with the class's purpose.

dydxwill added 30 commits July 30, 2024 11:34

first

bda8ec4

test

af1eb90

test

2c10c1d

revert

20e627f

merge

0dd93fd

update proto

cd6572a

lint

dc2bc8e

rm global cache

3c7a63b

dev5

af79f20

add branch

96cb022

dev2

57e54fa

lint

ce15619

revert ghwf

e6fcbf5

fix

1357239

merge

2816d5f

add indexer proto

7d08ac2

address cmts

8d333b8

add cmts

390b118

Merge branch 'wl/sa_updates' of https://github.com/dydxprotocol/v4-chain

fd7abb0

into lw/sa2

t

318e9ac

latest

71c0f08

merge

f03735d

add su update

1ab0669

add snapshotting logic

079ffd5

updates

3d5db4e

set Snapshot to true

ee5e8f7

merge

71cbe99

add back todo

0a6bd3a

add gh wf

cd7472c

add logs

ef9a0ec

jonfung-dydx reviewed Aug 9, 2024

View reviewed changes

address cmts

53bb1be

dydxwill requested a review from jonfung-dydx August 12, 2024 16:17

coderabbitai bot reviewed Aug 12, 2024

View reviewed changes

jonfung-dydx reviewed Aug 12, 2024

View reviewed changes

jonfung-dydx approved these changes Aug 12, 2024

View reviewed changes

dydxwill added 2 commits August 12, 2024 13:43

address cmts

320b3b0

Merge branch 'main' of https://github.com/dydxprotocol/v4-chain into …

edd63c7

…wl/sa2

coderabbitai bot reviewed Aug 12, 2024

View reviewed changes

gh wf

65b28cc

dydxwill merged commit 021e4e3 into main Aug 12, 2024
32 of 33 checks passed

dydxwill deleted the wl/sa2 branch August 12, 2024 18:26

jonfung-dydx pushed a commit that referenced this pull request Aug 15, 2024

FNS subaccount implementation (#2059)

3f76b54

jonfung-dydx pushed a commit that referenced this pull request Sep 17, 2024

FNS subaccount implementation (#2059)

d2ef3a3

jonfung-dydx pushed a commit that referenced this pull request Sep 26, 2024

FNS subaccount implementation (#2059)

a07b8ae

coderabbitai bot mentioned this pull request Sep 27, 2024

bug: Internalize FNS logic to stage FinalizeBlock events #2399

Merged

6 tasks

jonfung-dydx pushed a commit that referenced this pull request Oct 4, 2024

FNS subaccount implementation (#2059)

7932bf2

jonfung-dydx pushed a commit that referenced this pull request Oct 18, 2024

FNS subaccount implementation (#2059)

0805c93

coderabbitai bot mentioned this pull request Oct 28, 2024

Full node streaming -- perp position to signed int #2544

Merged

coderabbitai bot mentioned this pull request Nov 21, 2024

[CT-1321] subscribe to market prices streaming services #2592

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FNS subaccount implementation #2059

FNS subaccount implementation #2059

dydxwill commented Aug 7, 2024 •

edited by coderabbitai bot

Loading

jonfung-dydx Aug 9, 2024

dydxwill Aug 12, 2024

jonfung-dydx Aug 9, 2024

dydxwill Aug 12, 2024

jonfung-dydx Aug 9, 2024

dydxwill Aug 12, 2024

jonfung-dydx Aug 9, 2024

dydxwill Aug 12, 2024

jonfung-dydx Aug 9, 2024

dydxwill Aug 12, 2024

jonfung-dydx Aug 9, 2024

dydxwill Aug 12, 2024

jonfung-dydx Aug 9, 2024 •

edited

Loading

dydxwill Aug 12, 2024

jonfung-dydx Aug 9, 2024

dydxwill Aug 12, 2024

jonfung-dydx Aug 12, 2024

jonfung-dydx Aug 12, 2024

dydxwill Aug 12, 2024

coderabbitai bot left a comment

jonfung-dydx Aug 12, 2024 •

edited

Loading

dydxwill Aug 12, 2024

jonfung-dydx Aug 12, 2024

jonfung-dydx Aug 12, 2024

jonfung-dydx Aug 12, 2024

dydxwill Aug 12, 2024

jonfung-dydx Aug 12, 2024

dydxwill Aug 12, 2024

coderabbitai bot left a comment

		@@ -544,6 +718,14 @@ func (sm *FullNodeStreamingManagerImpl) InitializeNewStreams(
		}

		sm.SendSnapshot(allUpdates, subscriptionId, blockHeight, execMode)

FNS subaccount implementation #2059

FNS subaccount implementation #2059

Conversation

dydxwill commented Aug 7, 2024 • edited by coderabbitai bot Loading

Changelist

Test Plan

Author/Reviewer Checklist

Summary by CodeRabbit

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonfung-dydx Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

jonfung-dydx Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

dydxwill commented Aug 7, 2024 •

edited by coderabbitai bot

Loading

jonfung-dydx Aug 9, 2024 •

edited

Loading

jonfung-dydx Aug 12, 2024 •

edited

Loading