initial implementation #3

oddaf · 2025-02-14T03:05:44Z

No description provided.

amusingaxl

❌ Found one major issue that allows bypassing step the way it's implemented.
ℹ️ Some additional suggestions are applicable.

script/input/1/README.md

src/DSPC.sol

Co-authored-by: amusingaxl <[email protected]> Signed-off-by: oddaf <[email protected]>

src/DSPC.sol

0xBasset · 2025-02-19T12:28:15Z

src/deployment/DSPCInstance.sol

What is this contract for?

It's just a standalone struct that can be imported when the spell onboarding.
See an example here.

amusingaxl

Left some additional suggestions for the deploy/init scripts.

src/deployment/DSPCInstance.sol

src/deployment/DSPCInit.sol

src/deployment/DSPCDeploy.sol

src/deployment/DSPCInit.sol

Co-authored-by: amusingaxl <[email protected]> Signed-off-by: oddaf <[email protected]>

… casting

amusingaxl · 2025-02-20T14:15:45Z

src/deployment/DSPCInit.sol

@@ -29,11 +33,11 @@ library DSPCInit {
     * @param inst The DSCP instance.
     */
    function init(DssInstance memory dss, DSPCInstance memory inst) internal {
-        inst.dspc.rely(address(inst.mom));
-        inst.mom.setAuthority(dss.chainlog.getAddress("MCD_ADM"));
+        RelayLike(inst.dspc).rely(address(inst.mom));


Suggested change

RelayLike(inst.dspc).rely(address(inst.mom));

RelyLike(inst.dspc).rely(address(inst.mom));

Done in 75211ed.

amusingaxl · 2025-02-20T14:15:55Z

src/deployment/DSPCInit.sol


        dss.jug.rely(address(inst.dspc));
        dss.pot.rely(address(inst.dspc));
-        SUSDSLike(dss.chainlog.getAddress("SUSDS")).rely(address(inst.dspc));
+        RelayLike(dss.chainlog.getAddress("SUSDS")).rely(address(inst.dspc));


Suggested change

RelayLike(dss.chainlog.getAddress("SUSDS")).rely(address(inst.dspc));

RelyLike(dss.chainlog.getAddress("SUSDS")).rely(address(inst.dspc));

Done in 75211ed.

src/DSPC.sol

Co-authored-by: amusingaxl <[email protected]> Signed-off-by: oddaf <[email protected]>

amusingaxl

Left some suggestions.

README.md

amusingaxl · 2025-02-24T21:24:23Z

src/DSPC.sol

+    uint256 public bad;
+    /// @notice Cooldown period between rate changes in seconds
+    uint256 public tau;
+    /// @notice Last time when rates were updated (Unix timestamp)
+    uint256 public toc;


We can make those fit into the same storage slot.
They are all going to be read in the same call, so we save a bit on gas.

Suggested change

uint256 public bad;

/// @notice Cooldown period between rate changes in seconds

uint256 public tau;

/// @notice Last time when rates were updated (Unix timestamp)

uint256 public toc;

uint8 public bad;

/// @notice Cooldown period between rate changes in seconds

uint64 public tau;

/// @notice Last time when rates were updated (Unix timestamp)

uint128 public toc;

In order to save on storage we're gonna have to store them all in a struct. Is that ok?

src/DSPC.sol

Co-authored-by: amusingaxl <[email protected]> Signed-off-by: oddaf <[email protected]>

amusingaxl · 2025-02-25T20:48:31Z

src/deployment/DSPCInit.sol

+    function init(DssInstance memory dss, DSPCInstance memory inst) internal {
+        RelyLike(inst.dspc).rely(address(inst.mom));
+        MomLike(inst.mom).setAuthority(dss.chainlog.getAddress("MCD_ADM"));
+
+        dss.jug.rely(address(inst.dspc));
+        dss.pot.rely(address(inst.dspc));
+        RelyLike(dss.chainlog.getAddress("SUSDS")).rely(address(inst.dspc));
+    }


Realized now that we're missing the initialization of the parameters, both global (i.e.: tau) and per-rate ones (i.e.: min, max, step).

Done in a47d4ff.

amusingaxl

Left some suggestions for the init script to be more inline with spell crafting style.

amusingaxl · 2025-03-05T16:04:22Z

src/deployment/DSPCInit.sol

+    uint256 tau;
+    /// @dev Time delay between rate updates


Suggested change

uint256 tau;

/// @dev Time delay between rate updates

/// @dev Time delay between rate updates

uint256 tau;

amusingaxl · 2025-03-05T16:06:57Z

src/deployment/DSPCInit.sol

+    /// @dev Minimum rate in basis points [0-65535]
+    uint16 max;
+    /// @dev Maximum rate in basis points [0-65535]


The range here is not relevant because:

It can be implicitly derived from the uint16 type

$655.35\%$ is not meaningful in this context.

Suggested change

/// @dev Minimum rate in basis points [0-65535]

uint16 max;

/// @dev Maximum rate in basis points [0-65535]

/// @dev Minimum rate in basis points

uint16 max;

/// @dev Maximum rate in basis points

amusingaxl · 2025-03-05T16:08:08Z

src/deployment/DSPCInit.sol

+struct DSPCConfig {
+    uint256 tau;
+    /// @dev Time delay between rate updates
+    DSPCIlkConfig[] ilks;


Missing natspec here.

amusingaxl · 2025-03-05T16:10:47Z

src/deployment/DSPCInit.sol

+        // Authorize DSPC in core contracts
+        dss.jug.rely(inst.dspc);
+        dss.pot.rely(inst.dspc);
+        DSPCLike(dss.chainlog.getAddress("SUSDS")).rely(inst.dspc);


This interface name doesn't make a lot of sense.

Suggested change

DSPCLike(dss.chainlog.getAddress("SUSDS")).rely(inst.dspc);

RelyLike(dss.chainlog.getAddress("SUSDS")).rely(inst.dspc);

amusingaxl · 2025-03-05T16:11:34Z

src/deployment/DSPCInit.sol

+        DSPCLike dspc = DSPCLike(inst.dspc);
+        DSPCMomLike mom = DSPCMomLike(inst.mom);


IMO it's cleaner if we don't add the casting here and just use inline casting. Otherwise, we're referencing the same values using 2 different names throughout the script.

Suggested change

DSPCLike dspc = DSPCLike(inst.dspc);

DSPCMomLike mom = DSPCMomLike(inst.mom);

amusingaxl · 2025-03-05T16:15:36Z

src/deployment/DSPCInit.sol

+interface DSPCLike {
+    function rely(address usr) external;
+    function file(bytes32 what, uint256 data) external;
+    function file(bytes32 ilk, bytes32 what, uint256 data) external;
+    function kiss(address usr) external;


Suggested change

interface DSPCLike {

function rely(address usr) external;

function file(bytes32 what, uint256 data) external;

function file(bytes32 ilk, bytes32 what, uint256 data) external;

function kiss(address usr) external;

interface RelyLike {

function rely(address usr) external;

}

interface DSPCLike is RelyLike {

function file(bytes32 what, uint256 data) external;

function file(bytes32 ilk, bytes32 what, uint256 data) external;

function kiss(address usr) external;

amusingaxl · 2025-03-05T16:17:33Z

src/deployment/DSPCInit.sol

+/// @dev Used to configure rate parameters for a specific collateral
+struct DSPCIlkConfig {
+    bytes32 ilk;
+    /// @dev Collateral identifier


Suggested change

/// @dev Collateral identifier

/// @dev Rate identifier

amusingaxl · 2025-03-05T16:18:18Z

src/deployment/DSPCInit.sol

+    bytes32 ilk;
+    /// @dev Collateral identifier
+    uint16 min;
+    /// @dev Minimum rate in basis points [0-65535]
+    uint16 max;
+    /// @dev Maximum rate in basis points [0-65535]
+    uint16 step;
+}
+/// @dev Step size in basis points [0-65535]


Not sure if this is intentional, but you're adding the natspec AFTER the corresponding line. I'd expect it to come before.

amusingaxl · 2025-03-05T16:18:53Z

src/deployment/DSPCInit.sol

+/// @title Global configuration parameters for DSPC
+/// @dev Used to configure global parameters and collateral-specific settings
+struct DSPCConfig {
+    uint256 tau;


Missing the setup of the buds:

Suggested change

uint256 tau;

uint256 tau;

address[] buds;

Potentially this could be 1 single address, as I don't think we'll ever onboard 2 facilitator wallets at once.

amusingaxl · 2025-03-05T16:20:32Z

src/deployment/DSPCDeploy.sol

+        // First set authority for DSPCMom
+        DSPCMom(inst.mom).setAuthority(params.owner);


This should be part of the initialization script.

Suggested change

// First set authority for DSPCMom

DSPCMom(inst.mom).setAuthority(params.owner);

initial implementation

e62ac29

oddaf requested review from amusingaxl and 0xp3th1um February 14, 2025 03:05

amusingaxl requested changes Feb 17, 2025

View reviewed changes

oddaf and others added 10 commits February 17, 2025 23:45

Update script/input/1/README.md

13e9d95

Co-authored-by: amusingaxl <[email protected]> Signed-off-by: oddaf <[email protected]>

Update src/DSPC.sol

2d4ff02

Co-authored-by: amusingaxl <[email protected]> Signed-off-by: oddaf <[email protected]>

Update src/DSPC.sol

2bccdbe

Co-authored-by: amusingaxl <[email protected]> Signed-off-by: oddaf <[email protected]>

Update src/DSPC.sol

2c3f886

Co-authored-by: amusingaxl <[email protected]> Signed-off-by: oddaf <[email protected]>

Update src/DSPC.sol

3c60b1f

Co-authored-by: amusingaxl <[email protected]> Signed-off-by: oddaf <[email protected]>

refactor: rename function put to set

52a59f7

fix: add check if data can be safely cast to uint16 in file()

c04ef6c

refactor: remove step restriction

d6ff178

chore: forge fmt

b341255

refactor: Update Set event to be emitted for each rate update

917eb4d

0xBasset reviewed Feb 19, 2025

View reviewed changes

amusingaxl requested changes Feb 19, 2025

View reviewed changes

oddaf and others added 9 commits February 19, 2025 14:46

Update src/deployment/DSPCInstance.sol

def70fa

Co-authored-by: amusingaxl <[email protected]> Signed-off-by: oddaf <[email protected]>

Update src/deployment/DSPCInit.sol

9c2898d

Co-authored-by: amusingaxl <[email protected]> Signed-off-by: oddaf <[email protected]>

Update src/deployment/DSPCInit.sol

b6c57c0

Co-authored-by: amusingaxl <[email protected]> Signed-off-by: oddaf <[email protected]>

Update src/deployment/DSPCDeploy.sol

253af36

Co-authored-by: amusingaxl <[email protected]> Signed-off-by: oddaf <[email protected]>

Update src/deployment/DSPCDeploy.sol

45a1834

Co-authored-by: amusingaxl <[email protected]> Signed-off-by: oddaf <[email protected]>

Update src/deployment/DSPCDeploy.sol

cec6dc4

Co-authored-by: amusingaxl <[email protected]> Signed-off-by: oddaf <[email protected]>

Update src/deployment/DSPCInit.sol

48b668a

Co-authored-by: amusingaxl <[email protected]> Signed-off-by: oddaf <[email protected]>

refactor: use addresses instead of interfaces in DSPCInstance to ease…

b9482c3

… casting

fix: enforce min < max and max > min on file()

2274abe

amusingaxl requested changes Feb 20, 2025

View reviewed changes

amusingaxl reviewed Feb 21, 2025

View reviewed changes

src/DSPC.sol Show resolved Hide resolved

oddaf and others added 3 commits February 21, 2025 12:48

fix: typo in RelyLike

75211ed

Update src/DSPC.sol

3d3a515

Co-authored-by: amusingaxl <[email protected]> Signed-off-by: oddaf <[email protected]>

feat: dspc cooldown + step

b7dadb5

oddaf added 2 commits February 24, 2025 14:24

remove unnecessary setup from dspcMom test

0582668

chore: forge fmt

e03048f

amusingaxl requested changes Feb 24, 2025

View reviewed changes

oddaf and others added 3 commits February 24, 2025 20:11

Apply suggestions from code review

d685819

Co-authored-by: amusingaxl <[email protected]> Signed-off-by: oddaf <[email protected]>

fix: revert message in test

9dcf622

chore: forge fmt

0a79139

amusingaxl reviewed Feb 25, 2025

View reviewed changes

feat: improvements to initialization script

a47d4ff

amusingaxl requested changes Mar 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

initial implementation #3

initial implementation #3

oddaf commented Feb 14, 2025

amusingaxl left a comment

0xBasset Feb 19, 2025

amusingaxl Feb 19, 2025

amusingaxl left a comment

amusingaxl Feb 20, 2025

oddaf Feb 21, 2025

amusingaxl Feb 20, 2025

oddaf Feb 21, 2025

amusingaxl left a comment

amusingaxl Feb 24, 2025

oddaf Feb 25, 2025

amusingaxl Feb 25, 2025

oddaf Feb 26, 2025

amusingaxl left a comment

amusingaxl Mar 5, 2025

amusingaxl Mar 5, 2025

amusingaxl Mar 5, 2025

amusingaxl Mar 5, 2025

amusingaxl Mar 5, 2025

amusingaxl Mar 5, 2025

amusingaxl Mar 5, 2025

amusingaxl Mar 5, 2025

amusingaxl Mar 5, 2025

amusingaxl Mar 5, 2025

amusingaxl Mar 5, 2025

	RelayLike(inst.dspc).rely(address(inst.mom));
	RelyLike(inst.dspc).rely(address(inst.mom));

	RelayLike(dss.chainlog.getAddress("SUSDS")).rely(address(inst.dspc));
	RelyLike(dss.chainlog.getAddress("SUSDS")).rely(address(inst.dspc));

	DSPCLike(dss.chainlog.getAddress("SUSDS")).rely(inst.dspc);
	RelyLike(dss.chainlog.getAddress("SUSDS")).rely(inst.dspc);

		DSPCLike dspc = DSPCLike(inst.dspc);
		DSPCMomLike mom = DSPCMomLike(inst.mom);

		// First set authority for DSPCMom
		DSPCMom(inst.mom).setAuthority(params.owner);

initial implementation #3

Are you sure you want to change the base?

initial implementation #3

Conversation

oddaf commented Feb 14, 2025

amusingaxl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amusingaxl 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

Choose a reason for hiding this comment

amusingaxl 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

Choose a reason for hiding this comment

amusingaxl 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

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