From e51f4ce619ecf1cdaf87aa25fec8aa0f6e20c3e5 Mon Sep 17 00:00:00 2001 From: Aslan Tashtanov Date: Wed, 24 Apr 2024 10:17:33 -0400 Subject: [PATCH 1/5] add caps to Account, validate users --- deepbook/sources/deepbook.move | 10 +- deepbook/sources/pool/account.move | 149 +++++++++++++++++++++++++---- deepbook/sources/pool/pool.move | 41 ++++---- 3 files changed, 163 insertions(+), 37 deletions(-) diff --git a/deepbook/sources/deepbook.move b/deepbook/sources/deepbook.move index 61e7b0fa..0fbaab00 100644 --- a/deepbook/sources/deepbook.move +++ b/deepbook/sources/deepbook.move @@ -15,7 +15,7 @@ module deepbook::deepbook { use deepbook::{ state::State, pool::{Order, Pool, DEEP}, - account::Account, + account::{Account, TradeProof}, }; // POOL MANAGEMENT @@ -125,6 +125,7 @@ module deepbook::deepbook { public fun place_limit_order( pool: &mut Pool, account: &mut Account, + proof: &TradeProof, client_order_id: u64, price: u64, quantity: u64, @@ -135,6 +136,7 @@ module deepbook::deepbook { ): u128 { pool.place_limit_order( account, + proof, client_order_id, price, quantity, @@ -167,19 +169,21 @@ module deepbook::deepbook { public fun cancel_order( pool: &mut Pool, account: &mut Account, + proof: &TradeProof, client_order_id: u128, ctx: &mut TxContext, ): Order { - pool.cancel_order(account, client_order_id, ctx) + pool.cancel_order(account, proof, client_order_id, ctx) } /// Public facing function to cancel all orders. public fun cancel_all_orders( pool: &mut Pool, account: &mut Account, + proof: &TradeProof, ctx: &mut TxContext, ): vector { - pool.cancel_all(account, ctx) + pool.cancel_all(account, proof, ctx) } /// Public facing function to get open orders for a user. diff --git a/deepbook/sources/pool/account.move b/deepbook/sources/pool/account.move index a49f920f..241f2760 100644 --- a/deepbook/sources/pool/account.move +++ b/deepbook/sources/pool/account.move @@ -7,9 +7,10 @@ // Is this intented to be a shared object or an owned object? Important: You cannot promote owned to shared! /// The Account is a shared object and holds all of the balances for a user. -/// It is passed into Pools for placing orders. All Pools can desposit and withdraw from the account. -/// When performing security checks, we need to ensure owned objects such as a Capability are not used. -/// Owned objects cause wallets to be locked when trading at a high frequency. +/// It is passed into Pools for placing orders. + + + module deepbook::account { use sui::{ bag::{Self, Bag}, @@ -17,41 +18,129 @@ module deepbook::account { coin::Coin, }; - //// The account doesn't have enough funds to be withdrawn. - const EAccountBalanceTooLow: u64 = 0; - /// The account doesn't have the balance. - const ENoBalance: u64 = 1; + const EInvalidOwner: u64 = 0; + const EInvalidTrader: u64 = 1; + const EInvalidProof: u64 = 2; + const EAccountBalanceTooLow: u64 = 3; + const ENoBalance: u64 = 4; + const EMaxTradeCapsReached: u64 = 5; + const EUserNotAllowListed: u64 = 6; + + const MAX_TRADE_CAPS: u64 = 10; - // TODO: use Bag instead of direct dynamic fields - /// Owned by user, this is what's passed into pools - public struct Account has key, store { + /// A shared object that's passed into Pools for placing orders. + public struct Account has key { id: UID, /// The owner of the account. owner: address, /// Stores the Coin Balances for this account. balances: Bag, + trade_cap_count: u64, + allow_listed: vector, } /// Identifier for balance public struct BalanceKey has store, copy, drop {} - /// Create an individual account + /// Owners of a TradeCap can mint TradeProofs. + public struct TradeCap has key, store { + id: UID, + account_id: ID, + } + + /// Owner and TradeCap owners can generate a TradeProof. + /// TradeProof is used to validate the account when trading on DeepBook. + public struct TradeProof has drop { + account_id: ID, + } + public fun new(ctx: &mut TxContext): Account { - // validate that this user hasn't reached account limit Account { id: object::new(ctx), owner: ctx.sender(), balances: bag::new(ctx), + trade_cap_count: 0, + allow_listed: vector[], + } + } + + public fun share(account: Account) { + transfer::share_object(account); + } + + /// Mint a TradeCap. Any owner of a TradeCap can mint a TradeProof, + /// which is used to validate the account when trading on DeepBook. + public fun mint_trade_cap(account: &mut Account, ctx: &mut TxContext): TradeCap { + account.validate_owner(ctx); + assert!(account.trade_cap_count < MAX_TRADE_CAPS, EMaxTradeCapsReached); + + let id = object::new(ctx); + account.allow_listed.push_back(id.to_inner()); + account.trade_cap_count = account.trade_cap_count + 1; + + TradeCap { + id, + account_id: object::id(account), + } + } + + /// Revoke a TradeCap. Only the owner can revoke a TradeCap. + public fun revoke_trade_cap(account: &mut Account, trade_cap_id: &ID, ctx: &TxContext) { + account.validate_owner(ctx); + let mut i = 0; + let len = account.allow_listed.length(); + while (i < len) { + if (&account.allow_listed[i] == trade_cap_id) { + account.allow_listed.swap_remove(i); + break; + }; + i = i + 1; + }; + + assert!(i < len, EUserNotAllowListed); + + account.trade_cap_count = account.trade_cap_count - 1; + } + + /// Generate a TradeProof by the owner. The owner does not pass a capability, + /// and can generate TradeProofs without the risk of equivocation. + public fun generate_proof_as_owner(account: &mut Account, ctx: &TxContext): TradeProof { + account.validate_owner(ctx); + + TradeProof { + account_id: object::id(account), + } + } + + /// Generate a TradeProof with a TradeCap. + /// Risk of equivocation since TradeCap is an owned object. + public fun generate_proof_as_trader(account: &mut Account, trade_cap: &TradeCap): TradeProof { + account.validate_trader(trade_cap); + + TradeProof { + account_id: object::id(account), } } - /// Deposit funds to an account. - /// TODO: security checks. - /// TODO: Pool can deposit. + /// Deposit funds to an account. Only owner can call this directly. public fun deposit( account: &mut Account, coin: Coin, + ctx: &mut TxContext, ) { + let proof = generate_proof_as_owner(account, ctx); + + account.deposit_with_proof(&proof, coin); + } + + /// Deposit funds to an account. Pool will call this to deposit funds. + public(package) fun deposit_with_proof( + account: &mut Account, + proof: &TradeProof, + coin: Coin, + ) { + proof.validate_proof(account); + let key = BalanceKey {}; let to_deposit = coin.into_balance(); @@ -63,14 +152,26 @@ module deepbook::account { } } - /// Withdraw funds from an account. - /// TODO: security checks. - /// TODO: Pool can withdraw. + /// Withdraw funds from an account. Only owner can call this directly. public fun withdraw( account: &mut Account, amount: u64, ctx: &mut TxContext, ): Coin { + let proof = generate_proof_as_owner(account, ctx); + + account.withdraw_with_proof(&proof, amount, ctx) + } + + /// Withdraw funds from an account. Pool will call this to withdraw funds. + public(package) fun withdraw_with_proof( + account: &mut Account, + proof: &TradeProof, + amount: u64, + ctx: &mut TxContext, + ): Coin { + proof.validate_proof(account); + let key = BalanceKey {}; assert!(account.balances.contains(key), ENoBalance); let acc_balance: &mut Balance = &mut account.balances[key]; @@ -83,4 +184,16 @@ module deepbook::account { public fun owner(account: &Account): address { account.owner } + + fun validate_owner(account: &Account, ctx: &TxContext) { + assert!(ctx.sender() == account.owner(), EInvalidOwner); + } + + fun validate_trader(account: &Account, trade_cap: &TradeCap) { + assert!(account.allow_listed.contains(object::borrow_id(trade_cap)), EInvalidTrader); + } + + fun validate_proof(proof: &TradeProof, account: &Account) { + assert!(object::id(account) == proof.account_id, EInvalidProof); + } } diff --git a/deepbook/sources/pool/pool.move b/deepbook/sources/pool/pool.move index 91d54a10..de1473d8 100644 --- a/deepbook/sources/pool/pool.move +++ b/deepbook/sources/pool/pool.move @@ -21,7 +21,7 @@ module deepbook::pool { pool_state::{Self, PoolState, PoolEpochState}, deep_price::{Self, DeepPrice}, big_vector::{Self, BigVector}, - account::Account, + account::{Account, TradeProof}, user::User, utils::{Self, encode_order_id}, math, @@ -160,6 +160,7 @@ module deepbook::pool { public(package) fun place_limit_order( self: &mut Pool, account: &mut Account, + proof: &TradeProof, client_order_id: u64, price: u64, quantity: u64, // in base asset @@ -198,9 +199,9 @@ module deepbook::pool { }; // Transfer Base, Quote, Fee to Pool. - if (base_quantity + base_fee > 0) self.deposit_base(account, base_quantity + base_fee, ctx); - if (quote_quantity + quote_fee > 0) self.deposit_quote(account, quote_quantity + quote_fee, ctx); - if (deep_fee > 0) self.deposit_deep(account, deep_fee, ctx); + if (base_quantity + base_fee > 0) self.deposit_base(account, proof, base_quantity + base_fee, ctx); + if (quote_quantity + quote_fee > 0) self.deposit_quote(account, proof, quote_quantity + quote_fee, ctx); + if (deep_fee > 0) self.deposit_deep(account, proof, deep_fee, ctx); let place_quantity = math::max(base_quantity, quote_quantity); let fee_quantity = math::max(math::max(base_fee, quote_fee), deep_fee); @@ -276,6 +277,7 @@ module deepbook::pool { public(package) fun cancel_order( self: &mut Pool, account: &mut Account, + proof: &TradeProof, order_id: u128, ctx: &mut TxContext, ): Order { @@ -290,25 +292,25 @@ module deepbook::pool { if (order_cancelled.is_bid) { // deposit quote asset back into user account let quote_asset_quantity = math::mul(order_cancelled.quantity, order_cancelled.price); - self.withdraw_quote(account, quote_asset_quantity, ctx) + self.withdraw_quote(account, proof, quote_asset_quantity, ctx) } else { // deposit base asset back into user account - self.withdraw_base(account, order_cancelled.quantity, ctx) + self.withdraw_base(account, proof, order_cancelled.quantity, ctx) }; // withdraw fees into user account // if pool is verified at the time of order placement, fees are in deepbook tokens if (order_cancelled.fee_is_deep) { // withdraw deepbook fees - self.withdraw_deep(account, order_cancelled.fee_quantity, ctx) + self.withdraw_deep(account, proof, order_cancelled.fee_quantity, ctx) } else if (order_cancelled.is_bid) { // withdraw quote asset fees // can be combined with withdrawal above, separate now for clarity - self.withdraw_quote(account, order_cancelled.fee_quantity, ctx) + self.withdraw_quote(account, proof, order_cancelled.fee_quantity, ctx) } else { // withdraw base asset fees // can be combined with withdrawal above, separate now for clarity - self.withdraw_base(account, order_cancelled.fee_quantity, ctx) + self.withdraw_base(account, proof, order_cancelled.fee_quantity, ctx) }; // Emit order cancelled event @@ -342,6 +344,7 @@ module deepbook::pool { public(package) fun cancel_all( self: &mut Pool, account: &mut Account, + proof: &TradeProof, ctx: &mut TxContext, ): vector{ let mut cancelled_orders = vector[]; @@ -352,7 +355,7 @@ module deepbook::pool { let mut i = 0; while (i < len) { let key = orders_vector[i]; - let cancelled_order = cancel_order(self, account, key, ctx); + let cancelled_order = cancel_order(self, account, proof, key, ctx); cancelled_orders.push_back(cancelled_order); i = i + 1; }; @@ -546,61 +549,67 @@ module deepbook::pool { fun deposit_base( self: &mut Pool, user_account: &mut Account, + proof: &TradeProof, amount: u64, ctx: &mut TxContext, ) { - let base = user_account.withdraw(amount, ctx); + let base = user_account.withdraw_with_proof(proof, amount, ctx); self.base_balances.join(base.into_balance()); } fun deposit_quote( self: &mut Pool, user_account: &mut Account, + proof: &TradeProof, amount: u64, ctx: &mut TxContext, ) { - let quote = user_account.withdraw(amount, ctx); + let quote = user_account.withdraw_with_proof(proof, amount, ctx); self.quote_balances.join(quote.into_balance()); } fun deposit_deep( self: &mut Pool, user_account: &mut Account, + proof: &TradeProof, amount: u64, ctx: &mut TxContext, ) { - let coin = user_account.withdraw(amount, ctx); + let coin = user_account.withdraw_with_proof(proof, amount, ctx); self.deepbook_balance.join(coin.into_balance()); } fun withdraw_base( self: &mut Pool, user_account: &mut Account, + proof: &TradeProof, amount: u64, ctx: &mut TxContext, ) { let coin = self.base_balances.split(amount).into_coin(ctx); - user_account.deposit(coin); + user_account.deposit_with_proof(proof, coin); } fun withdraw_quote( self: &mut Pool, user_account: &mut Account, + proof: &TradeProof, amount: u64, ctx: &mut TxContext, ) { let coin = self.quote_balances.split(amount).into_coin(ctx); - user_account.deposit(coin); + user_account.deposit_with_proof(proof, coin); } fun withdraw_deep( self: &mut Pool, user_account: &mut Account, + proof: &TradeProof, amount: u64, ctx: &mut TxContext, ) { let coin = self.deepbook_balance.split(amount).into_coin(ctx); - user_account.deposit(coin); + user_account.deposit_with_proof(proof, coin); } #[allow(unused_function)] From ca73947affa88c74b8f06fe25d156b059d358d1e Mon Sep 17 00:00:00 2001 From: Aslan Tashtanov Date: Wed, 24 Apr 2024 11:00:51 -0400 Subject: [PATCH 2/5] comments --- deepbook/sources/pool/account.move | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/deepbook/sources/pool/account.move b/deepbook/sources/pool/account.move index 241f2760..5b99ae2f 100644 --- a/deepbook/sources/pool/account.move +++ b/deepbook/sources/pool/account.move @@ -1,16 +1,11 @@ // Copyright (c) Mysten Labs, Inc. // SPDX-License-Identifier: Apache-2.0 -// TODO: I think it might make more sense to represent ownership by a "Capability", -// instead of by address. That allows for flexible access control (someone could wrap their AccountCap) -// and pass it to others. -// Is this intented to be a shared object or an owned object? Important: You cannot promote owned to shared! - -/// The Account is a shared object and holds all of the balances for a user. -/// It is passed into Pools for placing orders. - - - +/// The Account is a shared object that holds all of the balances for a user. A combination of Account and +/// TradeProof are passed into Pool to perform trades. A TradeProof can be generated in two ways: by the +/// owner directly, or by any TradeCap owner. The owner can generate a TradeProof without the risk of +/// equivocation. The TradeCap owner, due to it being an owned object, risks equivocation when generating +/// a TradeProof. Generally, a high frequency trading engine will trade as the default owner. module deepbook::account { use sui::{ bag::{Self, Bag}, From 39800509c8d8f61cf56a47494f763b55d74bf063 Mon Sep 17 00:00:00 2001 From: Aslan Tashtanov Date: Wed, 24 Apr 2024 20:57:14 -0400 Subject: [PATCH 3/5] comments --- deepbook/sources/pool/account.move | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-) diff --git a/deepbook/sources/pool/account.move b/deepbook/sources/pool/account.move index 5b99ae2f..56c023dc 100644 --- a/deepbook/sources/pool/account.move +++ b/deepbook/sources/pool/account.move @@ -19,7 +19,7 @@ module deepbook::account { const EAccountBalanceTooLow: u64 = 3; const ENoBalance: u64 = 4; const EMaxTradeCapsReached: u64 = 5; - const EUserNotAllowListed: u64 = 6; + const ETradeCapNotInList: u64 = 6; const MAX_TRADE_CAPS: u64 = 10; @@ -30,7 +30,6 @@ module deepbook::account { owner: address, /// Stores the Coin Balances for this account. balances: Bag, - trade_cap_count: u64, allow_listed: vector, } @@ -54,7 +53,6 @@ module deepbook::account { id: object::new(ctx), owner: ctx.sender(), balances: bag::new(ctx), - trade_cap_count: 0, allow_listed: vector[], } } @@ -67,11 +65,10 @@ module deepbook::account { /// which is used to validate the account when trading on DeepBook. public fun mint_trade_cap(account: &mut Account, ctx: &mut TxContext): TradeCap { account.validate_owner(ctx); - assert!(account.trade_cap_count < MAX_TRADE_CAPS, EMaxTradeCapsReached); + assert!(account.allow_listed.length() < MAX_TRADE_CAPS, EMaxTradeCapsReached); let id = object::new(ctx); account.allow_listed.push_back(id.to_inner()); - account.trade_cap_count = account.trade_cap_count + 1; TradeCap { id, @@ -82,19 +79,10 @@ module deepbook::account { /// Revoke a TradeCap. Only the owner can revoke a TradeCap. public fun revoke_trade_cap(account: &mut Account, trade_cap_id: &ID, ctx: &TxContext) { account.validate_owner(ctx); - let mut i = 0; - let len = account.allow_listed.length(); - while (i < len) { - if (&account.allow_listed[i] == trade_cap_id) { - account.allow_listed.swap_remove(i); - break; - }; - i = i + 1; - }; - - assert!(i < len, EUserNotAllowListed); - - account.trade_cap_count = account.trade_cap_count - 1; + + let (exists, idx) = account.allow_listed.index_of(trade_cap_id); + assert!(exists, ETradeCapNotInList); + account.allow_listed.swap_remove(idx); } /// Generate a TradeProof by the owner. The owner does not pass a capability, From 75ebfec910d9e2e7bebea0164ac2891744d12775 Mon Sep 17 00:00:00 2001 From: Aslan Tashtanov Date: Thu, 25 Apr 2024 06:47:36 -0400 Subject: [PATCH 4/5] comments --- deepbook/sources/pool/account.move | 35 ++++++++++++++---------------- 1 file changed, 16 insertions(+), 19 deletions(-) diff --git a/deepbook/sources/pool/account.move b/deepbook/sources/pool/account.move index 56c023dc..d37a8851 100644 --- a/deepbook/sources/pool/account.move +++ b/deepbook/sources/pool/account.move @@ -1,11 +1,11 @@ // Copyright (c) Mysten Labs, Inc. // SPDX-License-Identifier: Apache-2.0 -/// The Account is a shared object that holds all of the balances for a user. A combination of Account and -/// TradeProof are passed into Pool to perform trades. A TradeProof can be generated in two ways: by the -/// owner directly, or by any TradeCap owner. The owner can generate a TradeProof without the risk of -/// equivocation. The TradeCap owner, due to it being an owned object, risks equivocation when generating -/// a TradeProof. Generally, a high frequency trading engine will trade as the default owner. +/// The Account is a shared object that holds all of the balances for a user. A combination of `Account` and +/// `TradeProof` are passed into a pool to perform trades. A `TradeProof` can be generated in two ways: by the +/// owner directly, or by any `TradeCap` owner. The owner can generate a `TradeProof` without the risk of +/// equivocation. The `TradeCap` owner, due to it being an owned object, risks equivocation when generating +/// a `TradeProof`. Generally, a high frequency trading engine will trade as the default owner. module deepbook::account { use sui::{ bag::{Self, Bag}, @@ -23,27 +23,25 @@ module deepbook::account { const MAX_TRADE_CAPS: u64 = 10; - /// A shared object that's passed into Pools for placing orders. + /// A shared object that is passed into pools for placing orders. public struct Account has key { id: UID, - /// The owner of the account. owner: address, - /// Stores the Coin Balances for this account. balances: Bag, allow_listed: vector, } - /// Identifier for balance + /// Balance identifier. public struct BalanceKey has store, copy, drop {} - /// Owners of a TradeCap can mint TradeProofs. + /// Owners of a `TradeCap` need to get a `TradeProof` to trade across pools in a single PTB (drops after). public struct TradeCap has key, store { id: UID, account_id: ID, } - /// Owner and TradeCap owners can generate a TradeProof. - /// TradeProof is used to validate the account when trading on DeepBook. + /// Account owner and `TradeCap` owners can generate a `TradeProof`. + /// `TradeProof` is used to validate the account when trading on DeepBook. public struct TradeProof has drop { account_id: ID, } @@ -61,8 +59,7 @@ module deepbook::account { transfer::share_object(account); } - /// Mint a TradeCap. Any owner of a TradeCap can mint a TradeProof, - /// which is used to validate the account when trading on DeepBook. + /// Mint a `TradeCap`, only owner can mint a `TradeCap`. public fun mint_trade_cap(account: &mut Account, ctx: &mut TxContext): TradeCap { account.validate_owner(ctx); assert!(account.allow_listed.length() < MAX_TRADE_CAPS, EMaxTradeCapsReached); @@ -76,7 +73,7 @@ module deepbook::account { } } - /// Revoke a TradeCap. Only the owner can revoke a TradeCap. + /// Revoke a `TradeCap`. Only the owner can revoke a `TradeCap`. public fun revoke_trade_cap(account: &mut Account, trade_cap_id: &ID, ctx: &TxContext) { account.validate_owner(ctx); @@ -85,7 +82,7 @@ module deepbook::account { account.allow_listed.swap_remove(idx); } - /// Generate a TradeProof by the owner. The owner does not pass a capability, + /// Generate a `TradeProof` by the owner. The owner does not require a capability /// and can generate TradeProofs without the risk of equivocation. public fun generate_proof_as_owner(account: &mut Account, ctx: &TxContext): TradeProof { account.validate_owner(ctx); @@ -95,8 +92,8 @@ module deepbook::account { } } - /// Generate a TradeProof with a TradeCap. - /// Risk of equivocation since TradeCap is an owned object. + /// Generate a `TradeProof` with a `TradeCap`. + /// Risk of equivocation since `TradeCap` is an owned object. public fun generate_proof_as_trader(account: &mut Account, trade_cap: &TradeCap): TradeProof { account.validate_trader(trade_cap); @@ -163,7 +160,7 @@ module deepbook::account { acc_balance.split(amount).into_coin(ctx) } - /// Returns the owner of the account + /// Returns the owner of the account. public fun owner(account: &Account): address { account.owner } From 19d7514ad0cf50da98ab3b33b8ed09eb5641c949 Mon Sep 17 00:00:00 2001 From: Aslan Tashtanov Date: Thu, 25 Apr 2024 17:35:57 -0400 Subject: [PATCH 5/5] increase trade cap limit --- deepbook/sources/pool/account.move | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/deepbook/sources/pool/account.move b/deepbook/sources/pool/account.move index d37a8851..18184291 100644 --- a/deepbook/sources/pool/account.move +++ b/deepbook/sources/pool/account.move @@ -21,7 +21,7 @@ module deepbook::account { const EMaxTradeCapsReached: u64 = 5; const ETradeCapNotInList: u64 = 6; - const MAX_TRADE_CAPS: u64 = 10; + const MAX_TRADE_CAPS: u64 = 1000; /// A shared object that is passed into pools for placing orders. public struct Account has key {