Sip 366 implementation #2145

leomassazza · 2024-05-09T17:28:57Z

No description provided.

…io/synthetix-v3 into SIP-366-implementation

noahlitvin · 2024-06-04T14:27:01Z

protocol/synthetix/contracts/interfaces/IVaultModule.sol

@@ -56,14 +164,181 @@ interface IVaultModule {
     *
     * Emits a {DelegationUpdated} event.
     */
-    function delegateCollateral(


Is it possible to have the interfaces set up where atomic delegation can still be possible and the interface doesn't change? Like this function could be the "process" function if necessary a delay is set?

There's also an abstraction where everything is async, and atomic can be done with a batch of two tx's when the delay is configured to 0.

i believe switching the delay and window to 0 leads to atomic processing with the same interface.

i have same feedback

hey @dbeal-eth can the legacy market migration be retrofitted for this sip? Want to avoid a situation of having unexpected behavior for sips that are still in limbo

dbeal-eth

please see my storage optimization comments in particular.

the removal of minDelegationTime is likely to have ripple effects elsewhere.

also, I already raised this with kaleb, but due to the removal of the minDelegationTime check on the PoolModule, it is no longer possible to enforce delegation time for users who want to frontrun on a pool level. We need to make sure the same rules apply both to pool owners, and to vault delegators.

dbeal-eth · 2024-06-04T16:11:30Z

protocol/synthetix/contracts/modules/core/MarketManagerModule.sol

+    /**
+     * @inheritdoc IMarketManagerModule
+     */
+    function setUndelegateCollateralWindow(


please combine these individual setUndelegateCollateral... functions into a single setter that takes a tuple/structure.

its likely that such values will be always set/changed at the same time, reduces surface area, and

same goes for getter.

dbeal-eth · 2024-06-04T16:21:15Z

protocol/synthetix/contracts/modules/core/PoolModule.sol

@@ -148,7 +148,6 @@ contract PoolModule is IPoolModule {
    ) external override {
        Pool.Data storage pool = Pool.loadExisting(poolId);
        Pool.onlyPoolOwner(poolId, ERC2771Context._msgSender());
-        pool.requireMinDelegationTimeElapsed(pool.lastConfigurationTime);


this line existed in order to prevent pool owners (or, single user-pool owners, which is something users can do, create a pool for themselves) from being exempt from the min delegation timeout

in order to effectively apply the delegation limitations to protect the pool, the same intent system must be developed on the market side as well.

In reference to #2145 (comment) hopefully a min delay / window can be enforced on pool owners, similar to the delay

dbeal-eth · 2024-06-04T16:24:55Z

protocol/synthetix/contracts/storage/AccountDelegationIntents.sol

+    /**
+     * @dev Returns the account delegation intents stored at the specified account id. Checks if it's valid
+     */
+    function getValid(uint128 accountId) internal returns (Data storage accountDelegationIntents) {


the name of this function and understanding the difference between getValid and loadValid seems extremely difficult. maybe a better name is loadWithInit or something?

dbeal-eth · 2024-06-04T16:32:57Z

protocol/synthetix/contracts/storage/DelegationIntent.sol

+        /**
+         * @notice An incrementing id (nonce) to ensure the  uniqueness of the intent and prevent replay attacks
+         */
+        uint256 id;


this field could be done without--see other comments

delegation is already very expensive so every storage slot we can reduce is huge help

dbeal-eth · 2024-06-04T16:39:57Z

protocol/synthetix/contracts/storage/AccountDelegationIntents.sol

+        uint128 accountId;
+        uint128 delegationIntentsEpoch; // nonce used to nuke previous intents using a new era (useful on liquidations)
+        SetUtil.UintSet intentsId;
+        mapping(bytes32 => SetUtil.UintSet) intentsByPair; // poolId/collateralType => intentIds[]


can we just embed DelegationIntent.Data[] type right here? then we:

don't need to store intentsId

don't need to store id inside of the DelegationIntent

since intents are added in timestamp order, we can wipe out expired intents very cleanly without searching the full intents list by creating another field lastActiveIntent and incrementing that.

if we make it a rule that intents have to be processed in the order they are submitted, this same field can be used to invalidate intents as we go

also if an intent is "cancelled" by the owner before it can be processed, we can add a field to the intent "cancelled, which only incurs a cost on intent processing and nowhere else

All in favor optimizing the removal of id @dbeal-eth and replacing with timestamps, in case the lp/account-id/market is the key to holding the stored intents... In order to avoid different timestamps stepping over each other. My concern is multiple users executing actions in the same block however, for vaults...

Regarding 3) yeah there is this expireIntent where you give it an id, and allows to expire specific intents, so no need to search for the full list, and there are a lot of view functions leo put together to find expired intents

The rule (mentioned in 4) to make intents processed in the order they are submitted can lead to issues with vaults... Assume the delay / window are configured differently for delegate / undelegate and a user specifies a undelegation on a vault and another user a delegate.. They can lead to stepping over each other... So this would require 2 intent data's right? One for delegation and another for undelegation?
Also another scenario I was thinking about is assume you have an intent that can't be expired or executed, and is in limbo until expiry, Example hitting caps on delegation. This leads to kind of bricking until parameters are updated... So enforcing sequentiality I thought about this and opted to not ask for it to be employed as to allow of execution of intents regardless of the order they are submitted....

yea 3 is literally intended tobe a simplification for clients by allowing for the contract to be able to handle everything effieicntly.

regarding 4, if delegate/undelegate have different window or delay, then we can separate the lists by "operation" (its already separated by vault). Tho to simplify things I dont see a lot of use in allowing different window limits for delegate/undelegate...

regarding update of parameters... I mean these settings apply immediately to all pending intents regardless of when they are created and seems like it would be an issue with either scheme, so seems like a small issue if anything comparing to the benefits of the optimizations.

There is also the situation of an intent that is neither executable and the window is not out for it's expiry, blocking the vault from executing any other intent. Aside from parameter changes, there are things that could go wrong and requiring sequential execution for async intents I feel is too fragile for the sake of optimization.

dbeal-eth · 2024-06-04T16:40:47Z

protocol/synthetix/contracts/storage/AccountDelegationIntents.sol

+        mapping(bytes32 => SetUtil.UintSet) intentsByPair; // poolId/collateralType => intentIds[]
+        // accounting for the intents collateral delegated
+        // Per Collateral
+        SetUtil.AddressSet delegatedCollaterals;


if we can avoid storing this and only track it off-chain, save about 2-3 storage slots!

not sure if this one is possible tho

dbeal-eth · 2024-06-04T16:42:49Z

protocol/synthetix/contracts/storage/AccountDelegationIntents.sol

+    /**
+     * @dev Returns the account delegation intents stored at the specified account id.
+     */
+    function load(


if we got rid of this load function and instead embedded this data structure into the Account, it would reduce load complexity and heiarchally store data in a convenient manner

kaleb-keny · 2024-06-05T09:48:21Z

please see my storage optimization comments in particular.

the removal of minDelegationTime is likely to have ripple effects elsewhere.

also, I already raised this with kaleb, but due to the removal of the minDelegationTime check on the PoolModule, it is no longer possible to enforce delegation time for users who want to frontrun on a pool level. We need to make sure the same rules apply both to pool owners, and to vault delegators.

Hey @leomassazza , there needs to be a minimum delay / window enforced on pool owners as db mentioned, otherwise one can setup a pool with 0 delay/window parameters in order to front-run price updates. That said, if the protocol owner, can push any parameters regardless of the minimum specified, on specific LP's, it would be great, as it allows for special cases where 0 delay/window are possible. But this isn't necessary in the near term and can be thought about in the future, dealing with just the minimum delay/window is fine for now.

Hey @dbeal-eth thank just to be on the same page here, you are fine with having a minimum delay / window applied on pool owners by the core right, in order to do away with minDelegateTime?

dbeal-eth · 2024-06-06T06:10:57Z

Hey @dbeal-eth thank just to be on the same page here, you are fine with having a minimum delay / window applied on pool owners by the core right, in order to do away with minDelegateTime?

yea I think this is fine. but I am way more concerned about the breaking interface changes. should not be made lightly...

kaleb-keny · 2024-06-06T08:29:31Z

Hey @dbeal-eth thank just to be on the same page here, you are fine with having a minimum delay / window applied on pool owners by the core right, in order to do away with minDelegateTime?

yea I think this is fine. but I am way more concerned about the breaking interface changes. should not be made lightly...

Awesome thank you, yeah I get you, given that the core staking is yet to be integrated across different protocols I think we can risk it at this point, giving dhedge the necessary time to prepare. The reason i insist on this is that the intention is to lower the intent delay to 30 minutes and window to 1 hour. So having a minDelegateTime set to zero if it's kept. Which might lead to issues in having to deal with legacy code there, in case someone can setup their own pool without intents and just use the old interface for delegation, with 0 minDelegateTime...

mjlescano · 2024-06-06T12:48:16Z

protocol/synthetix/contracts/storage/Market.sol

        uint32 __reservedForLater1;
        uint64 __reservedForLater2;
-        uint64 __reservedForLater3;
-        uint64 __reservedForLater4;


Listing storage changes as this looks risky:

⚠️ Deprecating uint32 minDelegateTime as uint32 __unusedLegacyStorageSlot (This should be marked with something like "DO NOT USE", because maybe it has data).

✅ Renaming uint32 __reservedForLater1 to uint32 undelegateCollateralDelay

✅ Expanding slot uint64 __reservedForLater3 to uint32 undelegateCollateralWindow & uint32 delegateCollateralDelay

❗ Renaming and Resizing uint64 __reservedForLater4 to uint32 delegateCollateralWindow. Please confirm @leomassazza, but this needs to add a uint32 __reservedForLaterX so it doesn't leak into the next slot for uint256 minLiquidityRatioD18

protocol/synthetix/contracts/storage/VaultEpoch.sol

* renames and comments on storage changes * reduce setter/getter footprint * apply same fix to mock market * fix test related to SetDelegateCollateralConfiguration * Missing fix on tests * rollback `minDelegationTime` and `delegateCollateral` removal * Add separate FF for legacy and two steps delegation * use twoStepDelegateCollateral in cannonfile test * fix test (attempt) * uses right FF * test both modes * remove duplicated code * update storage * rename getValid * add global min/max to delegate delay and window * update storage dump * Add test to both FFs enabled * Link `AccountDelegationIntents` directly to `Account` * add tests to global params * use declarationTime as id for intents * Revert "use declarationTime as id for intents" This reverts commit beb4f8c. * fix typo in comments

dbeal-eth

seems ok

we need to do some gas profiling tho because the way this has been designed may work well enough to cover the somewhat edge case of the needs of dhedge, but it does so by using an extreme amoun tof gas and storing excessive amounts of data imo. I am very certain there is a better way.

dbeal-eth · 2024-06-29T23:59:21Z

protocol/synthetix/contracts/storage/Market.sol

+        uint32 delegateCollateralDelay; // Accumulated Alignment 128
+        uint32 delegateCollateralWindow; // Accumulated Alignment 160
+        uint32 __reservedForLater1; // Accumulated Alignment 192
+        uint64 __reservedForLater2; // Accumulated Alignment 256


it seems reasonable to believe that these alignments are correct but have we verified with the actual solidity compiler? I'm told that it creates a storage manifest cc @mjlescano

dbeal-eth · 2024-06-30T00:05:34Z

protocol/synthetix/contracts/modules/core/VaultModule.sol


    bytes32 private constant _DELEGATE_FEATURE_FLAG = "delegateCollateral";
+    bytes32 private constant _TWO_STEPS_DELEGATE_FEATURE_FLAG = "twoStepsDelegateCollateral";


really we should be naming all feature flags the same as the function they guard

bytes32 private constant _DECLARE_DELEGATE_FEATURE_FLAG = "declareIntentToDelegateCollateral"; bytes32 private constant _PROCESS_DELEGATE_FEATURE_FLAG = "processIntentToDelegateCollateral";

having a flag to disable both of them at the same time icnraseses code complexity and can be done almost as eaily by simply specifying both of the function names for deny all feature flag.
that way its straightforward in an emergency situation to block any function by simply using its name.

fixed, though couldn't use the full function name due to naming length (bytes32)

leomassazza added 4 commits May 8, 2024 14:42

wip sip366

2209136

wip 366

45dfd41

wip 366 start fixing tests

cebc9c4

fix, wrong params for pair

f41d109

leomassazza self-assigned this May 9, 2024

leomassazza added 16 commits May 13, 2024 11:23

wip: add per collateral accounting

e3ab470

Test MarketManagerModule

18de031

Fix LiquidationModule test

de561f0

wip: fix tests

1f49264

checkpoint: fixing vault module test (wip)

5a28067

checkpoint: use delta for intent declarations

13a5120

checkpoint: added some events

b2cb01c

Link to liquidation

7280c58

checkpoint: more tests fixed and bugs smashed

64030e3

Fix tests

3f68d81

fix other project tests

c60b1c4

fix another tests on bfp

6ae5345

Only owner can delete all intents

8289f11

Make window time flexible with configuration changes

c6b5332

wip: timing tests

55cf924

test edge cases

383d27e

leomassazza marked this pull request as ready for review May 20, 2024 20:48

leomassazza added 8 commits May 21, 2024 11:55

Reduce AccountDelegationIntents footprint

18a4308

Cleanup and comments

a47d06c

update storage.dump

12db077

add skipped test - multiple markets with diff timing

1027f7f

deps fix

3a4ac69

undo deps fix

ed9d256

Merge branch 'SIP-366-implementation' of https://github.com/Synthetix…

fc9cf62

…io/synthetix-v3 into SIP-366-implementation

fix deps

f0531e2

some fixes

05cd81f

noahlitvin reviewed Jun 4, 2024

View reviewed changes

dbeal-eth requested changes Jun 4, 2024

View reviewed changes

add views test + small fix

2e5ddcd

mjlescano requested changes Jun 6, 2024

View reviewed changes

leomassazza added 8 commits June 26, 2024 12:48

Merge branch 'main' into SIP-366-implementation

cb02ac7

fix bfp tests

4061dec

more fixes on bfp (roll back changes)

f9ba8da

add missing empty line

cc0bd12

add more info to view

62de774

fix wrong sign + a small refactor

9a44955

fix test

6272a49

dbeal-eth approved these changes Jun 30, 2024

View reviewed changes

leomassazza added 13 commits July 1, 2024 11:46

rename FFs

ac3ad55

fix tests

c11d354

Merge branch 'main' into SIP-366-implementation

c4fa5d3

push storage dump

0a2d08e

Merge branch 'main' into SIP-366-implementation

b190bfc

Merge branch 'main' into SIP-366-implementation

dde2c7d

Reduce delegationIntent footprint

4ff64db

fix missing references to intent.id

cd8a7f9

Merge branch 'main' into SIP-366-implementation

eb05eba

Merge branch 'main' into SIP-366-implementation

1f46654

fix test

f960f81

Merge branch 'main' into SIP-366-implementation

712921c

Merge branch 'main' into SIP-366-implementation

dcb9ea4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sip 366 implementation #2145

Sip 366 implementation #2145

leomassazza commented May 9, 2024

noahlitvin Jun 4, 2024

kaleb-keny Jun 4, 2024 •

edited

Loading

dbeal-eth Jun 4, 2024

kaleb-keny Jun 5, 2024

dbeal-eth left a comment •

edited

Loading

dbeal-eth Jun 4, 2024

dbeal-eth Jun 4, 2024

kaleb-keny Jun 5, 2024

dbeal-eth Jun 4, 2024

dbeal-eth Jun 4, 2024

dbeal-eth Jun 4, 2024

kaleb-keny Jun 5, 2024 •

edited

Loading

dbeal-eth Jun 6, 2024 •

edited

Loading

kaleb-keny Jun 6, 2024

dbeal-eth Jun 4, 2024

dbeal-eth Jun 4, 2024

dbeal-eth Jun 4, 2024

kaleb-keny commented Jun 5, 2024 •

edited

Loading

dbeal-eth commented Jun 6, 2024

kaleb-keny commented Jun 6, 2024 •

edited

Loading

mjlescano Jun 6, 2024 •

edited

Loading

dbeal-eth left a comment

dbeal-eth Jun 29, 2024

dbeal-eth Jun 30, 2024

leomassazza Jul 2, 2024


		bytes32 private constant _DELEGATE_FEATURE_FLAG = "delegateCollateral";
		bytes32 private constant _TWO_STEPS_DELEGATE_FEATURE_FLAG = "twoStepsDelegateCollateral";

Sip 366 implementation #2145

Are you sure you want to change the base?

Sip 366 implementation #2145

Conversation

leomassazza commented May 9, 2024

Choose a reason for hiding this comment

kaleb-keny Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbeal-eth left a comment • 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

kaleb-keny Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

dbeal-eth Jun 6, 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

kaleb-keny commented Jun 5, 2024 • edited Loading

dbeal-eth commented Jun 6, 2024

kaleb-keny commented Jun 6, 2024 • edited Loading

mjlescano Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

dbeal-eth left a 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

kaleb-keny Jun 4, 2024 •

edited

Loading

dbeal-eth left a comment •

edited

Loading

kaleb-keny Jun 5, 2024 •

edited

Loading

dbeal-eth Jun 6, 2024 •

edited

Loading

kaleb-keny commented Jun 5, 2024 •

edited

Loading

kaleb-keny commented Jun 6, 2024 •

edited

Loading

mjlescano Jun 6, 2024 •

edited

Loading