Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[CLOB-1043] replicate IsLiquidatable logic on daemon #873

Merged
merged 11 commits into from
Dec 19, 2023
Merged

[CLOB-1043] replicate IsLiquidatable logic on daemon #873

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
d446a89
[CLOB-1043] replicate IsLiquidatable logic on daemon
jayy04 Dec 10, 2023
d9fa678
Merge branch 'main' into jy/clob-1043
jayy04 Dec 11, 2023
7180bd1
comments
jayy04 Dec 11, 2023
6a36196
Merge branch 'main' into jy/clob-1043
jayy04 Dec 12, 2023
1d38b49
comments
jayy04 Dec 12, 2023
d12f799
update to return error
jayy04 Dec 12, 2023
85bb561
[CLOB-1044] replicate funding settlement on liquidation daemon (#882)
jayy04 Dec 13, 2023
1d7facb
Merge branch 'main' into jy/clob-1043
jayy04 Dec 13, 2023
577500c
[CLOB-1043] deprecate unused grpc query (#889)
jayy04 Dec 13, 2023
2294683
[CLOB-1046] populate negative tnc subaccounts in grpc request (#890)
jayy04 Dec 18, 2023
b90c55b
Merge branch 'main' into jy/clob-1043
jayy04 Dec 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 13 additions & 26 deletions protocol/daemons/flags/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,9 @@ const (
FlagBridgeDaemonLoopDelayMs = "bridge-daemon-loop-delay-ms"
FlagBridgeDaemonEthRpcEndpoint = "bridge-daemon-eth-rpc-endpoint"

FlagLiquidationDaemonEnabled = "liquidation-daemon-enabled"
FlagLiquidationDaemonLoopDelayMs = "liquidation-daemon-loop-delay-ms"
FlagLiquidationDaemonSubaccountPageLimit = "liquidation-daemon-subaccount-page-limit"
FlagLiquidationDaemonRequestChunkSize = "liquidation-daemon-request-chunk-size"
FlagLiquidationDaemonEnabled = "liquidation-daemon-enabled"
FlagLiquidationDaemonLoopDelayMs = "liquidation-daemon-loop-delay-ms"
FlagLiquidationDaemonQueryPageLimit = "liquidation-daemon-query-page-limit"
)

// Shared flags contains configuration flags shared by all daemons.
Expand Down Expand Up @@ -52,9 +51,8 @@ type LiquidationFlags struct {
Enabled bool
// LoopDelayMs configures the update frequency of the liquidation daemon.
LoopDelayMs uint32
// SubaccountPageLimit configures the pagination limit for fetching subaccounts.
SubaccountPageLimit uint64
RequestChunkSize uint64
// QueryPageLimit configures the pagination limit for fetching subaccounts.
QueryPageLimit uint64
}

// PriceFlags contains configuration flags for the Price Daemon.
Expand Down Expand Up @@ -90,10 +88,9 @@ func GetDefaultDaemonFlags() DaemonFlags {
EthRpcEndpoint: "",
},
Liquidation: LiquidationFlags{
Enabled: true,
LoopDelayMs: 1_600,
SubaccountPageLimit: 1_000,
RequestChunkSize: 50,
Enabled: true,
LoopDelayMs: 1_600,
QueryPageLimit: 1_000,
},
Price: PriceFlags{
Enabled: true,
Expand Down Expand Up @@ -160,14 +157,9 @@ func AddDaemonFlagsToCmd(
"Delay in milliseconds between running the Liquidation Daemon task loop.",
)
cmd.Flags().Uint64(
FlagLiquidationDaemonSubaccountPageLimit,
df.Liquidation.SubaccountPageLimit,
"Limit on the number of subaccounts to fetch per query in the Liquidation Daemon task loop.",
)
cmd.Flags().Uint64(
FlagLiquidationDaemonRequestChunkSize,
df.Liquidation.RequestChunkSize,
"Limit on the number of subaccounts per collateralization check in the Liquidation Daemon task loop.",
FlagLiquidationDaemonQueryPageLimit,
df.Liquidation.QueryPageLimit,
"Limit on the number of items to fetch per query in the Liquidation Daemon task loop.",
)

// Price Daemon.
Expand Down Expand Up @@ -235,14 +227,9 @@ func GetDaemonFlagValuesFromOptions(
result.Liquidation.LoopDelayMs = v
}
}
if option := appOpts.Get(FlagLiquidationDaemonSubaccountPageLimit); option != nil {
if v, err := cast.ToUint64E(option); err == nil {
result.Liquidation.SubaccountPageLimit = v
}
}
if option := appOpts.Get(FlagLiquidationDaemonRequestChunkSize); option != nil {
if option := appOpts.Get(FlagLiquidationDaemonQueryPageLimit); option != nil {
if v, err := cast.ToUint64E(option); err == nil {
result.Liquidation.RequestChunkSize = v
result.Liquidation.QueryPageLimit = v
}
}

Expand Down
8 changes: 3 additions & 5 deletions protocol/daemons/flags/flags_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ func TestAddDaemonFlagsToCmd(t *testing.T) {

flags.FlagLiquidationDaemonEnabled,
flags.FlagLiquidationDaemonLoopDelayMs,
flags.FlagLiquidationDaemonSubaccountPageLimit,
flags.FlagLiquidationDaemonQueryPageLimit,

flags.FlagPriceDaemonEnabled,
flags.FlagPriceDaemonLoopDelayMs,
Expand All @@ -52,8 +52,7 @@ func TestGetDaemonFlagValuesFromOptions_Custom(t *testing.T) {

optsMap[flags.FlagLiquidationDaemonEnabled] = true
optsMap[flags.FlagLiquidationDaemonLoopDelayMs] = uint32(2222)
optsMap[flags.FlagLiquidationDaemonSubaccountPageLimit] = uint64(3333)
optsMap[flags.FlagLiquidationDaemonRequestChunkSize] = uint64(4444)
optsMap[flags.FlagLiquidationDaemonQueryPageLimit] = uint64(3333)

optsMap[flags.FlagPriceDaemonEnabled] = true
optsMap[flags.FlagPriceDaemonLoopDelayMs] = uint32(4444)
Expand Down Expand Up @@ -83,8 +82,7 @@ func TestGetDaemonFlagValuesFromOptions_Custom(t *testing.T) {
// Liquidation Daemon.
require.Equal(t, optsMap[flags.FlagLiquidationDaemonEnabled], r.Liquidation.Enabled)
require.Equal(t, optsMap[flags.FlagLiquidationDaemonLoopDelayMs], r.Liquidation.LoopDelayMs)
require.Equal(t, optsMap[flags.FlagLiquidationDaemonSubaccountPageLimit], r.Liquidation.SubaccountPageLimit)
require.Equal(t, optsMap[flags.FlagLiquidationDaemonRequestChunkSize], r.Liquidation.RequestChunkSize)
require.Equal(t, optsMap[flags.FlagLiquidationDaemonQueryPageLimit], r.Liquidation.QueryPageLimit)

// Price Daemon.
require.Equal(t, optsMap[flags.FlagPriceDaemonEnabled], r.Price.Enabled)
Expand Down
183 changes: 0 additions & 183 deletions protocol/daemons/liquidation/client/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,20 +7,14 @@ import (
"testing"

"github.com/cometbft/cometbft/libs/log"
"github.com/cosmos/cosmos-sdk/types/query"
appflags "github.com/dydxprotocol/v4-chain/protocol/app/flags"
d_constants "github.com/dydxprotocol/v4-chain/protocol/daemons/constants"
"github.com/dydxprotocol/v4-chain/protocol/daemons/flags"
"github.com/dydxprotocol/v4-chain/protocol/daemons/liquidation/api"
"github.com/dydxprotocol/v4-chain/protocol/daemons/liquidation/client"
"github.com/dydxprotocol/v4-chain/protocol/mocks"
"github.com/dydxprotocol/v4-chain/protocol/testutil/appoptions"
"github.com/dydxprotocol/v4-chain/protocol/testutil/constants"
daemontestutils "github.com/dydxprotocol/v4-chain/protocol/testutil/daemons"
"github.com/dydxprotocol/v4-chain/protocol/testutil/grpc"
clobtypes "github.com/dydxprotocol/v4-chain/protocol/x/clob/types"
satypes "github.com/dydxprotocol/v4-chain/protocol/x/subaccounts/types"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
)

Expand Down Expand Up @@ -70,183 +64,6 @@ func TestStart_UnixSocketConnectionFails(t *testing.T) {
mockGrpcClient.AssertNumberOfCalls(t, "CloseConnection", 1)
}

func TestRunLiquidationDaemonTaskLoop(t *testing.T) {
jayy04 marked this conversation as resolved.
Show resolved Hide resolved
df := flags.GetDefaultDaemonFlags()
tests := map[string]struct {
// mocks
setupMocks func(ctx context.Context, mck *mocks.QueryClient)

// expectations
expectedLiquidatableSubaccountIds []satypes.SubaccountId
expectedError error
}{
"Success": {
setupMocks: func(ctx context.Context, mck *mocks.QueryClient) {
req := &satypes.QueryAllSubaccountRequest{
Pagination: &query.PageRequest{
Limit: df.Liquidation.SubaccountPageLimit,
},
}
response := &satypes.QuerySubaccountAllResponse{
Subaccount: []satypes.Subaccount{
constants.Carl_Num0_1BTC_Short,
constants.Dave_Num0_1BTC_Long_50000USD,
},
}
mck.On("SubaccountAll", ctx, req).Return(response, nil)

req2 := &clobtypes.AreSubaccountsLiquidatableRequest{
SubaccountIds: []satypes.SubaccountId{
constants.Carl_Num0,
constants.Dave_Num0,
},
}
response2 := &clobtypes.AreSubaccountsLiquidatableResponse{
Results: []clobtypes.AreSubaccountsLiquidatableResponse_Result{
{
SubaccountId: constants.Carl_Num0,
IsLiquidatable: true,
},
{
SubaccountId: constants.Dave_Num0,
IsLiquidatable: false,
},
},
}
mck.On("AreSubaccountsLiquidatable", ctx, req2).Return(response2, nil)

req3 := &api.LiquidateSubaccountsRequest{
SubaccountIds: []satypes.SubaccountId{
constants.Carl_Num0,
},
}
response3 := &api.LiquidateSubaccountsResponse{}
mck.On("LiquidateSubaccounts", ctx, req3).Return(response3, nil)
},
},
"Success - no open position": {
setupMocks: func(ctx context.Context, mck *mocks.QueryClient) {
req := &satypes.QueryAllSubaccountRequest{
Pagination: &query.PageRequest{
Limit: df.Liquidation.SubaccountPageLimit,
},
}
response := &satypes.QuerySubaccountAllResponse{
Subaccount: []satypes.Subaccount{
constants.Carl_Num0_599USD, // no open positions
constants.Dave_Num0_599USD, // no open positions
},
}
mck.On("SubaccountAll", ctx, req).Return(response, nil)
req2 := &api.LiquidateSubaccountsRequest{
SubaccountIds: []satypes.SubaccountId{},
}
response2 := &api.LiquidateSubaccountsResponse{}
mck.On("LiquidateSubaccounts", ctx, req2).Return(response2, nil)
},
},
"Success - no liquidatable subaccounts": {
setupMocks: func(ctx context.Context, mck *mocks.QueryClient) {
req := &satypes.QueryAllSubaccountRequest{
Pagination: &query.PageRequest{
Limit: df.Liquidation.SubaccountPageLimit,
},
}
response := &satypes.QuerySubaccountAllResponse{
Subaccount: []satypes.Subaccount{
constants.Carl_Num0_1BTC_Short,
constants.Dave_Num0_1BTC_Long_50000USD,
},
}
mck.On("SubaccountAll", ctx, req).Return(response, nil)

req2 := &clobtypes.AreSubaccountsLiquidatableRequest{
SubaccountIds: []satypes.SubaccountId{
constants.Carl_Num0,
constants.Dave_Num0,
},
}
response2 := &clobtypes.AreSubaccountsLiquidatableResponse{
Results: []clobtypes.AreSubaccountsLiquidatableResponse_Result{
{
SubaccountId: constants.Carl_Num0,
IsLiquidatable: false,
},
{
SubaccountId: constants.Dave_Num0,
IsLiquidatable: false,
},
},
}
mck.On("AreSubaccountsLiquidatable", ctx, req2).Return(response2, nil)
req3 := &api.LiquidateSubaccountsRequest{
SubaccountIds: []satypes.SubaccountId{},
}
response3 := &api.LiquidateSubaccountsResponse{}
mck.On("LiquidateSubaccounts", ctx, req3).Return(response3, nil)
},
},
"Panics on error - SubaccountAll": {
setupMocks: func(ctx context.Context, mck *mocks.QueryClient) {
mck.On("SubaccountAll", mock.Anything, mock.Anything).Return(nil, errors.New("test error"))
},
expectedError: errors.New("test error"),
},
"Panics on error - AreSubaccountsLiquidatable": {
setupMocks: func(ctx context.Context, mck *mocks.QueryClient) {
mck.On("SubaccountAll", mock.Anything, mock.Anything).Return(&satypes.QuerySubaccountAllResponse{
Subaccount: []satypes.Subaccount{
constants.Carl_Num0_1BTC_Short,
},
}, nil)
mck.On("AreSubaccountsLiquidatable", mock.Anything, mock.Anything).Return(nil, errors.New("test error"))
},
expectedError: errors.New("test error"),
},
"Panics on error - LiquidateSubaccounts": {
setupMocks: func(ctx context.Context, mck *mocks.QueryClient) {
mck.On("SubaccountAll", mock.Anything, mock.Anything).Return(&satypes.QuerySubaccountAllResponse{
Subaccount: []satypes.Subaccount{
constants.Carl_Num0_1BTC_Short,
},
}, nil,
)
mck.On("AreSubaccountsLiquidatable", mock.Anything, mock.Anything).Return(
&clobtypes.AreSubaccountsLiquidatableResponse{},
nil,
)
mck.On("LiquidateSubaccounts", mock.Anything, mock.Anything).Return(nil, errors.New("test error"))
},
expectedError: errors.New("test error"),
},
}

for name, tc := range tests {
t.Run(name, func(t *testing.T) {
queryClientMock := &mocks.QueryClient{}
tc.setupMocks(grpc.Ctx, queryClientMock)
s := client.SubTaskRunnerImpl{}

c := client.NewClient(log.NewNopLogger())
c.SubaccountQueryClient = queryClientMock
c.ClobQueryClient = queryClientMock
c.LiquidationServiceClient = queryClientMock

err := s.RunLiquidationDaemonTaskLoop(
grpc.Ctx,
c,
flags.GetDefaultDaemonFlags().Liquidation,
)
if tc.expectedError != nil {
require.EqualError(t, err, tc.expectedError.Error())
} else {
require.NoError(t, err)
queryClientMock.AssertExpectations(t)
}
})
}
}

// FakeSubTaskRunner is a mock implementation of the SubTaskRunner interface for testing.
type FakeSubTaskRunner struct {
err error
Expand Down
31 changes: 0 additions & 31 deletions protocol/daemons/liquidation/client/grpc_helper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import (
"github.com/dydxprotocol/v4-chain/protocol/daemons/liquidation/api"
"github.com/dydxprotocol/v4-chain/protocol/lib/metrics"
blocktimetypes "github.com/dydxprotocol/v4-chain/protocol/x/blocktime/types"
clobtypes "github.com/dydxprotocol/v4-chain/protocol/x/clob/types"
perptypes "github.com/dydxprotocol/v4-chain/protocol/x/perpetuals/types"
pricestypes "github.com/dydxprotocol/v4-chain/protocol/x/prices/types"
satypes "github.com/dydxprotocol/v4-chain/protocol/x/subaccounts/types"
Expand Down Expand Up @@ -45,7 +44,6 @@ func (c *Client) GetPreviousBlockInfo(
// GetAllPerpetuals queries gRPC server and returns a list of perpetuals.
func (c *Client) GetAllPerpetuals(
ctx context.Context,
blockHeight uint32,
pageLimit uint64,
) (
perpetuals []perptypes.Perpetual,
Comment on lines 46 to 51
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 NOTE
Codebase verification is a beta feature.

The results confirm that the metric constant CheckCollateralizationForSubaccounts is used in two places within the codebase:

  1. It is defined in protocol/lib/metrics/constants.go.
  2. It is used in protocol/daemons/liquidation/client/sub_task_runner.go within a defer statement for telemetry purposes.

The context provided around the usage in sub_task_runner.go indicates that the metric was used to measure the latency of the CheckCollateralizationForSubaccounts function. Since this function has been removed, the corresponding metric should also be removed or updated to reflect the new logic that replaces the removed function.

Given this information, the removal of the CheckCollateralizationForSubaccounts function does indeed require attention to the associated metric constant and its usage. The codebase should be updated to ensure that there are no orphaned references or incorrect telemetry measurements.

The review should therefore include the following points:

  • The metric constant CheckCollateralizationForSubaccounts in protocol/lib/metrics/constants.go should be removed or repurposed if it is no longer relevant.

  • The telemetry measurement in protocol/daemons/liquidation/client/sub_task_runner.go should be updated to remove the reference to the removed function or to measure the latency of the new logic that has replaced it.

  • Remove or update the metric constant CheckCollateralizationForSubaccounts in protocol/lib/metrics/constants.go.

  • Address the reference to CheckCollateralizationForSubaccounts in protocol/daemons/liquidation/client/sub_task_runner.go to ensure correct telemetry measurement.

Analysis chain

Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [44-201]

The CheckCollateralizationForSubaccounts function has been removed. Verify that there are no orphaned references to this function in the codebase and that its removal does not affect any dependent logic.

The shell script results indicate that there are references to the CheckCollateralizationForSubaccounts function in two files: protocol/lib/metrics/constants.go and protocol/daemons/liquidation/client/sub_task_runner.go. To verify that the removal of this function does not affect any dependent logic, we need to check these files to understand the context in which the function is referenced.

Let's start by examining the references in protocol/lib/metrics/constants.go to see if it's just a metric constant that needs to be removed or updated. Then we'll check protocol/daemons/liquidation/client/sub_task_runner.go to see how the function was being used and ensure that its removal has been properly handled in the logic of the sub task runner.

The results from the shell scripts show that the CheckCollateralizationForSubaccounts function is referenced in two places:

  1. In protocol/lib/metrics/constants.go, it appears to be a string constant named CheckCollateralizationForSubaccounts. This is likely used as a key or label for metrics and may need to be removed or updated if it is no longer relevant due to the removal of the function.

  2. In protocol/daemons/liquidation/client/sub_task_runner.go, the constant metrics.CheckCollateralizationForSubaccounts is used, which suggests that the function's removal should also prompt an update to the metrics collection logic in this file.

To ensure that the removal of the CheckCollateralizationForSubaccounts function is properly handled, we need to verify that:

  • The metric constant in protocol/lib/metrics/constants.go is either removed or no longer used.
  • The reference in protocol/daemons/liquidation/client/sub_task_runner.go is addressed, meaning that the code has been updated to remove the metric logging or it has been replaced with an alternative logic.

Let's proceed with checking the usage of the metric constant in protocol/lib/metrics/constants.go and how the reference in protocol/daemons/liquidation/client/sub_task_runner.go is handled after the removal of the CheckCollateralizationForSubaccounts function.

Scripts Executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for references to `CheckCollateralizationForSubaccounts` in the codebase.
rg 'CheckCollateralizationForSubaccounts' --files-with-matches

Length of output: 151

Script:

#!/bin/bash
# Check the reference in protocol/lib/metrics/constants.go
rg 'CheckCollateralizationForSubaccounts' protocol/lib/metrics/constants.go

# Check the reference in protocol/daemons/liquidation/client/sub_task_runner.go
rg 'CheckCollateralizationForSubaccounts' protocol/daemons/liquidation/client/sub_task_runner.go

Length of output: 302

Script:

#!/bin/bash
# Check if the metric constant is used elsewhere in the codebase
rg 'CheckCollateralizationForSubaccounts' --files-with-matches

# Examine the usage of the metric constant in sub_task_runner.go
rg 'CheckCollateralizationForSubaccounts' protocol/daemons/liquidation/client/sub_task_runner.go -A 5 -B 5

Length of output: 584

Expand Down Expand Up @@ -85,7 +83,6 @@ func (c *Client) GetAllPerpetuals(
// GetAllLiquidityTiers queries gRPC server and returns a list of liquidityTiers.
func (c *Client) GetAllLiquidityTiers(
ctx context.Context,
blockHeight uint32,
pageLimit uint64,
) (
liquidityTiers []perptypes.LiquidityTier,
Expand Down Expand Up @@ -125,7 +122,6 @@ func (c *Client) GetAllLiquidityTiers(
// GetAllMarketPrices queries gRPC server and returns a list of market prices.
func (c *Client) GetAllMarketPrices(
ctx context.Context,
blockHeight uint32,
pageLimit uint64,
) (
marketPrices []pricestypes.MarketPrice,
Expand Down Expand Up @@ -205,33 +201,6 @@ func (c *Client) GetAllSubaccounts(
return subaccounts, nil
}

// CheckCollateralizationForSubaccounts queries a gRPC server using `AreSubaccountsLiquidatable`
// and returns a list of collateralization statuses for the given list of subaccount ids.
func (c *Client) CheckCollateralizationForSubaccounts(
ctx context.Context,
subaccountIds []satypes.SubaccountId,
) (
results []clobtypes.AreSubaccountsLiquidatableResponse_Result,
err error,
) {
defer telemetry.ModuleMeasureSince(
metrics.LiquidationDaemon,
time.Now(),
metrics.CheckCollateralizationForSubaccounts,
metrics.Latency,
)

query := &clobtypes.AreSubaccountsLiquidatableRequest{
SubaccountIds: subaccountIds,
}
response, err := c.ClobQueryClient.AreSubaccountsLiquidatable(ctx, query)
if err != nil {
return nil, err
}

return response.Results, nil
}

// SendLiquidatableSubaccountIds sends a list of unique and potentially liquidatable
// subaccount ids to a gRPC server via `LiquidateSubaccounts`.
func (c *Client) SendLiquidatableSubaccountIds(
Expand Down
Loading
Loading