[1/N] Questions & nits #34

manolisliolios · 2024-04-18T21:17:22Z

No description provided.

deepbook/sources/pool/pool.move

0xaslan · 2024-04-19T10:02:59Z

deepbook/sources/pool/pool.move

@@ -502,6 +496,7 @@ module deepbook::pool {
    }

    /// Get the pool key string base+quote (if base, quote in lexicographic order) otherwise return quote+base
+    /// TODO: Why is this needed as a key? Why don't we just use the ID of the pool as an ID? 


When creating a pool, it is added to the pool registry in State using pool_key. We want to ensure that if you have created a SUI/USDC pool, then you can no longer create any more SUI/USDC or USDC/SUI pools. They should all be identifiable with the same pool_key. Perhaps it should be called something else?

Why don't we just save it with an object that is like PoolKey<phantom A,phantom B> and do these weird string manipulations?

The goal is to disallow the creation of duplicate and reversed pools. If that is a simpler way to do it then I am all for it

You could check for both the existence of PoolKey<A,B> as well as PoolKey<B,A>.
Want other people's thoughts too on this though!

0xaslan · 2024-04-19T10:03:36Z

deepbook/sources/pool/pool.move

@@ -545,6 +540,9 @@ module deepbook::pool {
        ctx: &mut TxContext,
    ) {
        // Withdraw from user account and merge into pool balances
+
+        // TOOD: Use constants with clear view of what it is instead of 0,1,2.
+        // Not sure if Enums will be a thing in time.


This is a placeholder for enums

But will Enums be available in time?

I can't say with high confidence

I see. Maybe we can design it thinking that they won't be and migrate to enums in case.

Enums won't hit mainnet until August, so we should assume it will not be available in time. Also, this is already updated in my PR here #33 which I will merge shortly.

0xaslan · 2024-04-19T10:04:44Z

deepbook/sources/pool/pool_state.move

@@ -13,6 +13,7 @@ module deepbook::pool_state {

    public struct PoolState has copy, store {
        epoch: u64,
+        // TODO: Won't this get too filled? Is there something planned to delete the historic states?


This will take 1 entry per epoch. A vector can hold 256 * 1024 entries. To maintain 10 years worth of historical data, we will make 3650 entries, 100x less than it can hold.

What about object size limit? we have a ceiling of 256KB.

A PoolEpochState has 6 u64s -> 48 bytes, we can round it up to 100. Let's say only 100KB is available for holding historic data, this would mean we can store ~3 years worth of epochs.

We need historic data for staked users who stopped trading but haven't claimed their rewards. When they click claim, they have to calculate the rewards for their last epoch, which can be many days ago. We would need to fetch the total traded volume of that epoch.

If all of the user's rewards have been calculated, then we no longer need the historic epoch data.

We can add a function to force calculate a user's rewards. Realistically, the number of such users is in the double digits, since they are market makers and not regular users.

Periodically we can call this cleanup function to bring all users up to the current epoch. Then we can cleanup the entire history.

We can run this cleanup every 6 months.

0xaslan · 2024-04-19T10:13:54Z

deepbook/sources/state/governance.move

@@ -43,6 +43,7 @@ module deepbook::governance {
    /// Governance struct that holds all the governance related data.
    /// This will reset during every epoch change, except voting_power which will be as needed.
    /// Participation is limited to users with staked voting power. vector and VecMap will not overflow.
+    /// Question: Why is this the case? (How do we know this?)


Assuming that similar to a vector this can hold ~100k entries, then the only way this reaches the limit is if there are 100k unique wallets in this specific pool that are attempting to vote. First I'll point out how unlikely this is since we have < 10, max 100 market makers.

Assuming someone is trying to attack the system, if we set the participation requirement to 10 DEEP tokens, then the attacker would need to acquire 1m DEEP tokens to make this overflow.

Furthermore, Governance resets every epoch, so this is not a data structure that accumulates data over time.

For guaranteed safety we can limit the number of participants to be 10k.

how was the 100K number derived here?

In the BigVector implementation I saw that a vector can hold 256 * 1024 elements. But the max object size you mentioned earlier, 256KB, is more restrictive

I think that's indeed close to 250KB/260KB (max max ceiling).

0xaslan · 2024-04-19T10:14:42Z

deepbook/sources/state/state.move

@@ -30,6 +30,7 @@ module deepbook::state {

    public struct State has key, store {
        id: UID,
+        // TODO: upgrade-ability plan? do we need?


Probably, we haven't touched on it yet.

leecchh · 2024-04-19T12:47:33Z

deepbook/sources/pool/account.move

+// 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!
+


We intend to create account as an owned object, and for users to pass the account object into different pools to share balances like USDC with SUI/USDC and USDT/USDC pools. Can we keep this functionality with a Cap approach? For example, if there is a master custodian users can deposit/withdraw from (a shared object), and passing caps into different pools all check that custodian for fund availability and update balances as necessary.

manolisliolios added 2 commits April 19, 2024 00:07

Adds a first round of nits and questions

25e8f8d

nits

536876a

manolisliolios requested review from leecchh, damirka and 0xaslan April 18, 2024 21:21

0xaslan reviewed Apr 19, 2024

View reviewed changes

deepbook/sources/pool/pool.move Show resolved Hide resolved

0xaslan reviewed Apr 19, 2024

View reviewed changes

leecchh reviewed Apr 19, 2024

View reviewed changes

Merge branch 'main' into ml/review-v1

7d2a6e9

leecchh merged commit e4131c1 into main Apr 19, 2024
3 checks passed

leecchh deleted the ml/review-v1 branch April 19, 2024 18:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1/N] Questions & nits #34

[1/N] Questions & nits #34

manolisliolios commented Apr 18, 2024

0xaslan Apr 19, 2024

manolisliolios Apr 19, 2024

0xaslan Apr 19, 2024

manolisliolios Apr 19, 2024

0xaslan Apr 19, 2024

manolisliolios Apr 19, 2024

0xaslan Apr 19, 2024

manolisliolios Apr 19, 2024

leecchh Apr 19, 2024

0xaslan Apr 19, 2024

manolisliolios Apr 19, 2024 •

edited

Loading

0xaslan Apr 19, 2024

0xaslan Apr 19, 2024

manolisliolios Apr 19, 2024

0xaslan Apr 19, 2024

manolisliolios Apr 19, 2024

0xaslan Apr 19, 2024

leecchh Apr 19, 2024 •

edited

Loading

[1/N] Questions & nits #34

[1/N] Questions & nits #34

Conversation

manolisliolios commented Apr 18, 2024

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

manolisliolios Apr 19, 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

leecchh Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

manolisliolios Apr 19, 2024 •

edited

Loading

leecchh Apr 19, 2024 •

edited

Loading