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

[TRA-84] Move SA module address transfers to use perpetual based SA accounts #1146

Merged
merged 6 commits into from
Mar 7, 2024
Merged

[TRA-84] Move SA module address transfers to use perpetual based SA accounts #1146

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8f36ca0
Move SA module address transfers to use perpetual based SA accounts
shrenujb Mar 6, 2024
7dfad36
slight refactor in apis
shrenujb Mar 6, 2024
4a3d396
fix perpetualId param
shrenujb Mar 6, 2024
4a7d3a9
remove isolated markets from sims until the full order flow is implem…
shrenujb Mar 6, 2024
78ebcda
address comments
shrenujb Mar 6, 2024
b03e4d4
Merge branch 'main' into tra84
shrenujb Mar 6, 2024
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
7 changes: 7 additions & 0 deletions protocol/testutil/constants/positions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,13 @@ var (
PerpetualId: 1,
Quantums: dtypes.NewIntFromBigInt(BigNegMaxUint64()), // 18,446,744,070 ETH, -$55,340,232,210,000 notional.
}
// Long position for arbitrary isolated market
PerpetualPosition_OneISOLong = satypes.PerpetualPosition{
PerpetualId: 3,
Quantums: dtypes.NewInt(100_000_000),
FundingIndex: dtypes.NewInt(0),
}

// Asset Positions
Usdc_Asset_0 = satypes.AssetPosition{
AssetId: 0,
Expand Down
2 changes: 1 addition & 1 deletion protocol/x/clob/abci_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1338,7 +1338,7 @@ func TestPrepareCheckState(t *testing.T) {
constants.Usdc.Denom,
).Return(sdk.NewCoin(constants.Usdc.Denom, sdkmath.NewIntFromBigInt(new(big.Int))))
mockBankKeeper.On(
"SendCoinsFromModuleToModule",
"SendCoins",
mock.Anything,
mock.Anything,
mock.Anything,
Comment on lines 1338 to 1344
Copy link
Contributor

Choose a reason for hiding this comment

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

📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [636-1339]

The test TestLiquidateSubaccounts thoroughly simulates the liquidation process with a detailed setup. However, it contains commented-out assertions at the end, suggesting that some verifications might be incomplete. Ensure that the test fully verifies the intended behavior by completing or updating these assertions.

Consider completing or updating the commented-out assertions at the end of the test to ensure comprehensive verification of the liquidation process.

📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1510-1773]

The test TestPrepareCheckState is comprehensive, simulating various states and checking for expected operations in the memclob. However, it contains commented-out assertions, suggesting that some verifications might be incomplete. Ensure that the test fully verifies the intended behavior by completing or updating these assertions.

Consider completing or updating the commented-out assertions to ensure comprehensive verification of the PrepareCheckState function's behavior.

Expand Down
28 changes: 21 additions & 7 deletions protocol/x/clob/keeper/liquidations_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,25 +235,25 @@ func TestPlacePerpetualLiquidation(t *testing.T) {
memClob := memclob.NewMemClobPriceTimePriority(false)
mockBankKeeper := &mocks.BankKeeper{}
mockBankKeeper.On(
"SendCoinsFromModuleToModule",
"SendCoins",
mock.Anything,
satypes.ModuleName,
authtypes.FeeCollectorName,
satypes.ModuleAddress,
authtypes.NewModuleAddress(authtypes.FeeCollectorName),
mock.Anything,
).Return(nil)
mockBankKeeper.On(
"SendCoins",
mock.Anything,
authtypes.NewModuleAddress(satypes.ModuleName),
authtypes.NewModuleAddress(perptypes.InsuranceFundName),
perptypes.InsuranceFundModuleAddress,
mock.Anything,
).Return(nil)
// Fee collector does not have any funds.
mockBankKeeper.On(
"SendCoinsFromModuleToModule",
"SendCoins",
mock.Anything,
authtypes.FeeCollectorName,
satypes.ModuleName,
authtypes.NewModuleAddress(authtypes.FeeCollectorName),
satypes.ModuleAddress,
mock.Anything,
).Return(sdkerrors.ErrInsufficientFunds)
// Give the insurance fund a 1M USDC balance.
Expand Down Expand Up @@ -793,6 +793,13 @@ func TestPlacePerpetualLiquidation_PreexistingLiquidation(t *testing.T) {
mock.Anything,
mock.Anything,
).Return(nil)
bk.On(
"SendCoins",
mock.Anything,
mock.Anything,
mock.Anything,
mock.Anything,
).Return(nil)
bk.On(
"GetBalance",
mock.Anything,
Expand Down Expand Up @@ -833,6 +840,13 @@ func TestPlacePerpetualLiquidation_PreexistingLiquidation(t *testing.T) {
mock.Anything,
mock.Anything,
).Return(nil)
bk.On(
"SendCoins",
mock.Anything,
mock.Anything,
mock.Anything,
mock.Anything,
).Return(nil)
bk.On(
"GetBalance",
mock.Anything,
Expand Down
7 changes: 7 additions & 0 deletions protocol/x/clob/keeper/mev_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1244,6 +1244,13 @@ func TestGetMidPrices(t *testing.T) {
mock.Anything,
mock.Anything,
).Return(nil)
mockBankKeeper.On(
"SendCoins",
mock.Anything,
mock.Anything,
mock.Anything,
mock.Anything,
).Return(nil)
Comment on lines +1247 to +1253
Copy link
Contributor

Choose a reason for hiding this comment

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

The SendCoins mock setup is present but not utilized within the TestGetMidPrices function. This could be a leftover from copy-pasting the setup code from another test function or an indication of an incomplete test scenario where SendCoins interaction was expected but not implemented. It's important to ensure that mock setups are relevant to the test cases being executed to avoid confusion and maintain clarity in test intentions.


ks := keepertest.NewClobKeepersTestContext(t, memclob, mockBankKeeper, indexer_manager.NewIndexerEventManagerNoop())
ctx := ks.Ctx.WithIsCheckTx(true)
Expand Down
4 changes: 2 additions & 2 deletions protocol/x/clob/keeper/orders_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -647,7 +647,7 @@ func TestPlaceShortTermOrder(t *testing.T) {
memClob := memclob.NewMemClobPriceTimePriority(false)
mockBankKeeper := &mocks.BankKeeper{}
mockBankKeeper.On(
"SendCoinsFromModuleToModule",
"SendCoins",
Copy link
Contributor

Choose a reason for hiding this comment

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

The mock setup for "SendCoins" in the TestPlaceShortTermOrder test function is overly broad and does not specify the expected arguments. This could lead to false positives in the test if the SendCoins function is called with unexpected arguments.

Consider specifying the expected arguments to ensure the test accurately reflects the intended behavior. For example:

mockBankKeeper.On(
  "SendCoins",
  mock.AnythingOfType("*context.Context"),
  expectedFromAddress,
  expectedToAddress,
  expectedAmount,
).Return(nil)

mock.Anything,
mock.Anything,
mock.Anything,
Comment on lines 647 to 653
Copy link
Contributor

Choose a reason for hiding this comment

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

📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [650-878]

In both TestPlaceShortTermOrder and TestAddPreexistingStatefulOrder, the mock.Anything matcher is used for all arguments in the SendCoins mock setup. This approach lacks specificity and could potentially mask issues if the SendCoins method is called with incorrect arguments during the test execution.

It's recommended to replace mock.Anything with more specific matchers or actual expected values to ensure the mock setup accurately reflects the expected behavior and arguments of the SendCoins method call.

Expand Down Expand Up @@ -875,7 +875,7 @@ func TestAddPreexistingStatefulOrder(t *testing.T) {
memClob := memclob.NewMemClobPriceTimePriority(false)
mockBankKeeper := &mocks.BankKeeper{}
mockBankKeeper.On(
"SendCoinsFromModuleToModule",
"SendCoins",
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to the previous comment, the mock setup for "SendCoins" in the TestAddPreexistingStatefulOrder test function is overly broad. Specifying expected arguments would improve the test's accuracy and reliability.

Adjust the mock setup to include expected arguments for better test accuracy.

mock.Anything,
mock.Anything,
mock.Anything,
Expand Down
94 changes: 47 additions & 47 deletions protocol/x/clob/keeper/process_operations_liquidations_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,17 +193,17 @@ func TestProcessProposerMatches_Liquidation_Success(t *testing.T) {
},
setupMockBankKeeper: func(bk *mocks.BankKeeper) {
bk.On(
"SendCoinsFromModuleToModule",
"SendCoins",
mock.Anything,
satypes.ModuleName,
authtypes.FeeCollectorName,
satypes.ModuleAddress,
authtypes.NewModuleAddress(authtypes.FeeCollectorName),
mock.MatchedBy(testutil_bank.MatchUsdcOfAmount(10_000_000)),
).Return(nil)
bk.On(
"SendCoins",
mock.Anything,
authtypes.NewModuleAddress(satypes.ModuleName),
authtypes.NewModuleAddress(perptypes.InsuranceFundName),
satypes.ModuleAddress,
perptypes.InsuranceFundModuleAddress,
// Subaccount pays $250 to insurance fund for liquidating 1 BTC.
mock.MatchedBy(testutil_bank.MatchUsdcOfAmount(250_000_000)),
).Return(nil).Once()
Expand Down Expand Up @@ -271,10 +271,10 @@ func TestProcessProposerMatches_Liquidation_Success(t *testing.T) {
},
setupMockBankKeeper: func(bk *mocks.BankKeeper) {
bk.On(
"SendCoinsFromModuleToModule",
"SendCoins",
mock.Anything,
satypes.ModuleName,
authtypes.FeeCollectorName,
satypes.ModuleAddress,
authtypes.NewModuleAddress(authtypes.FeeCollectorName),
mock.MatchedBy(testutil_bank.MatchUsdcOfAmount(10_100_000)),
).Return(nil)
bk.On(
Expand All @@ -286,8 +286,8 @@ func TestProcessProposerMatches_Liquidation_Success(t *testing.T) {
bk.On(
"SendCoins",
mock.Anything,
authtypes.NewModuleAddress(perptypes.InsuranceFundName),
authtypes.NewModuleAddress(satypes.ModuleName),
perptypes.InsuranceFundModuleAddress,
satypes.ModuleAddress,
// Insurance fund covers $1 loss for liquidating 1 BTC.
mock.MatchedBy(testutil_bank.MatchUsdcOfAmount(1_000_000)),
).Return(nil).Once()
Expand Down Expand Up @@ -357,17 +357,17 @@ func TestProcessProposerMatches_Liquidation_Success(t *testing.T) {
},
setupMockBankKeeper: func(bk *mocks.BankKeeper) {
bk.On(
"SendCoinsFromModuleToModule",
"SendCoins",
mock.Anything,
satypes.ModuleName,
authtypes.FeeCollectorName,
satypes.ModuleAddress,
authtypes.NewModuleAddress(authtypes.FeeCollectorName),
mock.MatchedBy(testutil_bank.MatchUsdcOfAmount(2_500_000)),
).Return(nil)
bk.On(
"SendCoins",
mock.Anything,
authtypes.NewModuleAddress(satypes.ModuleName),
authtypes.NewModuleAddress(perptypes.InsuranceFundName),
satypes.ModuleAddress,
perptypes.InsuranceFundModuleAddress,
// Subaccount pays $62.5 to insurance fund for liquidating 0.25 BTC.
mock.MatchedBy(testutil_bank.MatchUsdcOfAmount(62_500_000)),
).Return(nil).Twice()
Expand Down Expand Up @@ -455,10 +455,10 @@ func TestProcessProposerMatches_Liquidation_Success(t *testing.T) {
},
setupMockBankKeeper: func(bk *mocks.BankKeeper) {
bk.On(
"SendCoinsFromModuleToModule",
"SendCoins",
mock.Anything,
satypes.ModuleName,
authtypes.FeeCollectorName,
satypes.ModuleAddress,
authtypes.NewModuleAddress(authtypes.FeeCollectorName),
mock.MatchedBy(testutil_bank.MatchUsdcOfAmount(2_525_000)),
).Return(nil)
bk.On(
Expand All @@ -470,8 +470,8 @@ func TestProcessProposerMatches_Liquidation_Success(t *testing.T) {
bk.On(
"SendCoins",
mock.Anything,
authtypes.NewModuleAddress(perptypes.InsuranceFundName),
authtypes.NewModuleAddress(satypes.ModuleName),
perptypes.InsuranceFundModuleAddress,
satypes.ModuleAddress,
// Insurance fund covers $0.25 loss for liquidating 0.25 BTC.
mock.MatchedBy(testutil_bank.MatchUsdcOfAmount(250_000)),
).Return(nil).Twice()
Expand Down Expand Up @@ -560,10 +560,10 @@ func TestProcessProposerMatches_Liquidation_Success(t *testing.T) {
},
setupMockBankKeeper: func(bk *mocks.BankKeeper) {
bk.On(
"SendCoinsFromModuleToModule",
"SendCoins",
mock.Anything,
satypes.ModuleName,
authtypes.FeeCollectorName,
satypes.ModuleAddress,
authtypes.NewModuleAddress(authtypes.FeeCollectorName),
mock.Anything,
).Return(nil)
bk.On(
Expand All @@ -575,16 +575,16 @@ func TestProcessProposerMatches_Liquidation_Success(t *testing.T) {
bk.On(
"SendCoins",
mock.Anything,
authtypes.NewModuleAddress(satypes.ModuleName),
authtypes.NewModuleAddress(perptypes.InsuranceFundName),
satypes.ModuleAddress,
perptypes.InsuranceFundModuleAddress,
// Pays insurance fund $0.75 for liquidating 0.75 BTC.
mock.MatchedBy(testutil_bank.MatchUsdcOfAmount(750_000)),
).Return(nil).Once()
bk.On(
"SendCoins",
mock.Anything,
authtypes.NewModuleAddress(perptypes.InsuranceFundName),
authtypes.NewModuleAddress(satypes.ModuleName),
perptypes.InsuranceFundModuleAddress,
satypes.ModuleAddress,
// Insurance fund covers $0.25 loss for liquidating 0.25 BTC.
mock.MatchedBy(testutil_bank.MatchUsdcOfAmount(250_000)),
).Return(nil).Once()
Expand Down Expand Up @@ -664,10 +664,10 @@ func TestProcessProposerMatches_Liquidation_Success(t *testing.T) {
},
setupMockBankKeeper: func(bk *mocks.BankKeeper) {
bk.On(
"SendCoinsFromModuleToModule",
"SendCoins",
mock.Anything,
satypes.ModuleName,
authtypes.FeeCollectorName,
satypes.ModuleAddress,
authtypes.NewModuleAddress(authtypes.FeeCollectorName),
mock.Anything,
).Return(nil)
bk.On(
Expand All @@ -679,17 +679,17 @@ func TestProcessProposerMatches_Liquidation_Success(t *testing.T) {
bk.On(
"SendCoins",
mock.Anything,
authtypes.NewModuleAddress(satypes.ModuleName),
authtypes.NewModuleAddress(perptypes.InsuranceFundName),
satypes.ModuleAddress,
perptypes.InsuranceFundModuleAddress,
// Pays insurance fund $0.378735 (capped by MaxLiquidationFeePpm)
// for liquidating 0.75 BTC.
mock.MatchedBy(testutil_bank.MatchUsdcOfAmount(378_735)),
).Return(nil).Once()
bk.On(
"SendCoins",
mock.Anything,
authtypes.NewModuleAddress(satypes.ModuleName),
authtypes.NewModuleAddress(perptypes.InsuranceFundName),
satypes.ModuleAddress,
perptypes.InsuranceFundModuleAddress,
// Pays insurance fund $0.121265.
mock.MatchedBy(testutil_bank.MatchUsdcOfAmount(121_265)),
).Return(nil).Once()
Expand Down Expand Up @@ -777,17 +777,17 @@ func TestProcessProposerMatches_Liquidation_Success(t *testing.T) {
},
setupMockBankKeeper: func(bk *mocks.BankKeeper) {
bk.On(
"SendCoinsFromModuleToModule",
"SendCoins",
mock.Anything,
satypes.ModuleName,
authtypes.FeeCollectorName,
satypes.ModuleAddress,
authtypes.NewModuleAddress(authtypes.FeeCollectorName),
mock.MatchedBy(testutil_bank.MatchUsdcOfAmount(5_000_000)),
).Return(nil)
bk.On(
"SendCoins",
mock.Anything,
authtypes.NewModuleAddress(satypes.ModuleName),
authtypes.NewModuleAddress(perptypes.InsuranceFundName),
satypes.ModuleAddress,
perptypes.InsuranceFundModuleAddress,
// Subaccount pays $125 to insurance fund for liquidating 0.5 BTC.
mock.MatchedBy(testutil_bank.MatchUsdcOfAmount(125_000_000)),
).Return(nil).Once()
Expand Down Expand Up @@ -890,17 +890,17 @@ func TestProcessProposerMatches_Liquidation_Success(t *testing.T) {
},
setupMockBankKeeper: func(bk *mocks.BankKeeper) {
bk.On(
"SendCoinsFromModuleToModule",
"SendCoins",
mock.Anything,
satypes.ModuleName,
authtypes.FeeCollectorName,
satypes.ModuleAddress,
authtypes.NewModuleAddress(authtypes.FeeCollectorName),
mock.MatchedBy(testutil_bank.MatchUsdcOfAmount(1)),
).Return(nil)
bk.On(
"SendCoins",
mock.Anything,
authtypes.NewModuleAddress(satypes.ModuleName),
authtypes.NewModuleAddress(perptypes.InsuranceFundName),
satypes.ModuleAddress,
perptypes.InsuranceFundModuleAddress,
mock.MatchedBy(testutil_bank.MatchUsdcOfAmount(25)),
).Return(nil)
},
Expand Down Expand Up @@ -1263,17 +1263,17 @@ func TestProcessProposerMatches_Liquidation_Failure(t *testing.T) {
},
setupMockBankKeeper: func(bk *mocks.BankKeeper) {
bk.On(
"SendCoinsFromModuleToModule",
"SendCoins",
mock.Anything,
mock.Anything,
authtypes.FeeCollectorName,
authtypes.NewModuleAddress(authtypes.FeeCollectorName),
mock.Anything,
).Return(fmt.Errorf("transfer failed"))
bk.On(
"SendCoins",
mock.Anything,
mock.Anything,
authtypes.NewModuleAddress(perptypes.InsuranceFundName),
perptypes.InsuranceFundModuleAddress,
mock.Anything,
).Return(nil)
},
Expand Down
Loading
Loading