[TRA-134] construct simple vault orders #1206
Conversation
WalkthroughThe recent updates introduce mathematical rounding capabilities for 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 (
|
| ) | ||
|
|
||
| // TODO (TRA-118): store vault strategy constants in x/vault state. | ||
| const ( |
There was a problem hiding this comment.
some of the constants here will be used when strategy is more fully implemented
![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: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- protocol/lib/big_math.go (1 hunks)
- protocol/lib/big_math_test.go (1 hunks)
- protocol/x/vault/keeper/orders.go (2 hunks)
- protocol/x/vault/keeper/orders_test.go (1 hunks)
- protocol/x/vault/types/expected_keepers.go (2 hunks)
Files skipped from review as they are similar to previous changes (5)
- protocol/lib/big_math.go
- protocol/lib/big_math_test.go
- protocol/x/vault/keeper/orders.go
- protocol/x/vault/keeper/orders_test.go
- protocol/x/vault/types/expected_keepers.go
| Pair: constants.TestMarketParams[1].Pair, | ||
| Exponent: constants.TestMarketParams[1].Exponent, | ||
| MinExchanges: constants.TestMarketParams[1].MinExchanges, | ||
| MinPriceChangePpm: 4_200, // Set a high min price change to test spread calculation. |
There was a problem hiding this comment.
nit: Add this to the test cases instead if it's a variable to test against.
| // Expected orders. | ||
| expectedOrders []clobtypes.Order | ||
| }{ | ||
| "Get orders from Vault for Clob Pair 0": { |
There was a problem hiding this comment.
nit: Non-blocking, the test cases are quite verbose and probably can be simplified to hold several parameters about each order rather than all the entire orders / constants could be declared for the orders.
There was a problem hiding this comment.
yeah agreed. let me simplify these tests
protocol/x/vault/keeper/orders.go
Outdated
| GoodTilOneof: goodTilBlockTime, | ||
| } | ||
|
|
||
| orders = append(orders, ask, bid) |
There was a problem hiding this comment.
nit: Non-blocking, given you already know the number of layers, the slice could have been allocated with length 2*layers and this could be:
orders[2*i] = ask
orders[2*i+1] = bid
| func (k Keeper) GetVaultClobOrderClientId( | ||
| ctx sdk.Context, | ||
| side clobtypes.Order_Side, | ||
| layer uint8, |
There was a problem hiding this comment.
nit: layer = 0 is invalid, given layer always starts at 1?
There was a problem hiding this comment.
technically layer = 0 is okay for this function. it's up to other functions on what value of layer to pass in. adding a check on layer != 0 is ok but might make using this function a bit too complicated as parent func has to check whether there's an error
| }, | ||
| "Sell, Block Height Odd (negative), Layer 202": { | ||
| side: clobtypes.Order_SIDE_SELL, // 1<<31 | ||
| blockHeight: -678987, // 1<<30 |
There was a problem hiding this comment.
nit: Weird test case given this is definitely an invalid state for the ctx.BlockHeight to be negative. Non-blocking though, maybe note that somewhere?
There was a problem hiding this comment.
yeah just added this test case as blockHeight is an int64. wanted to make sure that a negative value doesn't break this function. will add a note on this
protocol/x/vault/keeper/orders.go
Outdated
| func (k Keeper) GetVaultClobOrders( | ||
| ctx sdk.Context, | ||
| vaultId types.VaultId, | ||
| ) (orders []clobtypes.Order) { |
There was a problem hiding this comment.
nit: Why not return error as well? Not obvious an error occurred if an empty slice of orders is returned.
There was a problem hiding this comment.
Good call! Will add error in return
protocol/x/vault/keeper/orders.go
Outdated
| func (k Keeper) GetVaultClobOrders( | ||
| ctx sdk.Context, | ||
| vaultId types.VaultId, | ||
| ) (orders []clobtypes.Order) { |
There was a problem hiding this comment.
minor nit: non-blocking, do you expect the orders returned to be passed around into other functions / helpers alot? Could make sense to return a slice of pointers to order structs.
There was a problem hiding this comment.
Good catch! A slice of pointers will be more efficient
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- protocol/x/vault/keeper/orders.go (2 hunks)
- protocol/x/vault/keeper/orders_test.go (1 hunks)
- protocol/x/vault/types/errors.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/x/vault/keeper/orders.go
- protocol/x/vault/keeper/orders_test.go
Additional comments: 2
protocol/x/vault/types/errors.go (2)
- 13-17: The addition of
ErrClobPairNotFoundis clear and follows the established pattern for error registration. The error code is unique and the message is descriptive.- 18-22: The addition of
ErrMarketParamNotFoundis clear and follows the established pattern for error registration. The error code is unique and the message is descriptive.
| func BigRatRoundToNearestMultiple( | ||
| value *big.Rat, | ||
| base uint32, | ||
| up bool, |
There was a problem hiding this comment.
nit: should we use an enum for this?
There was a problem hiding this comment.
I think a boolean should suffice?
| ) | ||
| quotientFloored := new(big.Int).Div(quotient.Num(), quotient.Denom()) | ||
|
|
||
| if up && quotientFloored.Cmp(quotient.Num()) != 0 { |
There was a problem hiding this comment.
if quotient.Num() == quotientFloored, doesnt that mean that quotient.Denom() = 1? Why does that mean we want to round up?
There was a problem hiding this comment.
quotientFloored.Cmp(quotient.Num()) != 0 is only true when quotient.Num() != quotientFloored (ref).
So this conditional is saying:
- if we want to round up (
up == true) and ifquotient.Num() != quotientFlooredthen add one to the floored quotient
So if quotient.Denom() == 1, then quotientFloored.Cmp(quotient.Num()) == 0 and this conditional wouldn't round up.
| const ( | ||
| // Determines how many layers of orders a vault places. | ||
| // E.g. if num_levels=2, a vault places 2 asks and 2 bids. | ||
| NUM_LAYERS = uint8(2) |
There was a problem hiding this comment.
why do we call this layers instead of max_orders_placed_by_vault_per_side. Does layers mean something in trading?
There was a problem hiding this comment.
oh it's just a made-up term. a vault places orders in terms of layers (layer 1, layer 2, ... where each layer gets further and further away from oracle price)
| spreadPpm := lib.Max( | ||
| MIN_BASE_SPREAD_PPM, | ||
| BASE_SPREAD_MIN_PRICE_CHANGE_PREMIUM_PPM+marketParam.MinPriceChangePpm, | ||
| ) |
There was a problem hiding this comment.
Why is this the spreadPpm?
There was a problem hiding this comment.
| // Construct ask at this layer. | ||
| ask := clobtypes.Order{ | ||
| OrderId: clobtypes.OrderId{ | ||
| SubaccountId: vault, | ||
| ClientId: k.GetVaultClobOrderClientId(ctx, clobtypes.Order_SIDE_SELL, uint8(i+1)), | ||
| OrderFlags: clobtypes.OrderIdFlags_LongTerm, | ||
| ClobPairId: clobPair.Id, | ||
| }, | ||
| Side: clobtypes.Order_SIDE_SELL, | ||
| Quantums: clobPair.StepBaseQuantums, // TODO (TRA-144): Implement order size | ||
| Subticks: lib.BigRatRoundToNearestMultiple( | ||
| askSubticks, | ||
| clobPair.SubticksPerTick, | ||
| true, // round up for asks | ||
| ), | ||
| GoodTilOneof: goodTilBlockTime, | ||
| } | ||
|
|
||
| // Construct bid at this layer. | ||
| bid := clobtypes.Order{ | ||
| OrderId: clobtypes.OrderId{ | ||
| SubaccountId: vault, | ||
| ClientId: k.GetVaultClobOrderClientId(ctx, clobtypes.Order_SIDE_BUY, uint8(i+1)), | ||
| OrderFlags: clobtypes.OrderIdFlags_LongTerm, | ||
| ClobPairId: clobPair.Id, | ||
| }, | ||
| Side: clobtypes.Order_SIDE_BUY, | ||
| Quantums: clobPair.StepBaseQuantums, // TODO (TRA-144): Implement order size | ||
| Subticks: lib.BigRatRoundToNearestMultiple( | ||
| bidSubticks, | ||
| clobPair.SubticksPerTick, | ||
| false, // round down for bids | ||
| ), | ||
| GoodTilOneof: goodTilBlockTime, | ||
| } | ||
|
|
||
| orders[2*i] = &ask | ||
| orders[2*i+1] = &bid | ||
| } |
There was a problem hiding this comment.
a lot of duplicate code here, could we move it into a subfunction and reuse?
There was a problem hiding this comment.
don't think it's too much duplicate code? I feel having ask and bid explicitly here makes it very clear what each order is like and keeps the function simple
Changelist
construct simple vault orders
Test Plan
unit tests
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.