Support Quark v2 in QuarkBuilder #83

kevincheng96 · 2024-09-23T20:04:01Z

Adds nonceSecret and replayCount to QuarkBuilder. For replayable operations, the QuarkBuilder will also generate the nonce using the nonceSecret and replayCount (see generateNonceFromSecret function).

We also uncover and fix a bug where 0 is being used as the nonce for an operation when a nonce is not provided for an account by the client. Now, the code reverts if a nonceSecret is not given for an account. We buff up the unit tests to assert the right values are being used for the nonces, since nonces were being checked previously.

kevincheng96 · 2024-09-23T22:53:48Z

test/builder/QuarkBuilderCometWithdraw.t.sol

+            // We need to set Bob as the withdrawer because only he has an account on chain 8453
+            cometWithdraw_({
+                chainId: 8453,
+                comet: cometUsdc_(8453),
+                assetSymbol: "LINK",
+                amount: 5e18,
+                withdrawer: address(0xb0b)
+            }),


Example of the nonce bug in action. Previously, we were withdrawing using 0xa11ce as the withdrawer on chain 8453, even though a nonce for 0xa11ce wasn't provided for chain 8453. Now, we have to set the withdrawer as 0xb0b since only he has a nonce on chain 8453.

hayesgm · 2024-09-23T23:52:08Z

src/builder/Accounts.sol

@@ -182,16 +181,17 @@ library Accounts {
        return findAssetPositions(assetAddress, chainAccounts.assetPositionsList);
    }

-    function findQuarkState(address account, Accounts.QuarkState[] memory quarkStates)
+    function findQuarkSecret(address account, Accounts.QuarkSecret[] memory quarkSecrets)


Do we need to do it this way? Wouldn't just "next random" work?

Synced offline on this. I don't think we resolved this discussion yet, but here's my response:

I considered specifying nonceSecrets on a non-account basis, but I decided to keep it account-based for a few reasons.

It conforms to the current interface that the client is passing in.

It reduces the complexity of the QuarkBuilder having to pick out nonceSecrets and keep track of which ones are already assigned to which operations.

It's kind of nice that this account-based approach guarantees there will always be enough nonceSecrets to construct the quark operations. It'll be one less thing that could go wrong to worry about in the QuarkBuilder

TBF, if we needed to do two operations on the same chain, this strategy wouldn't work? E.g. let's say we wanted to make two delayed operations on the same chain.

~~If we have two operations on the same chain for the same account, we wrap those in a multicall operation. So having a single nonceSecret for an account on each chain is guaranteed to be sufficient.~~

EDIT: I see your point. If there are two operations that have to be separate, then this wouldn't work. Good point. I can work on the "next random" approach in a follow-up PR to see how complicated it is

hayesgm · 2024-09-23T23:52:29Z

src/builder/Actions.sol

@@ -58,6 +58,9 @@ library Actions {
    uint256 constant SWAP_EXPIRY_BUFFER = 3 days;
    uint256 constant TRANSFER_EXPIRY_BUFFER = 7 days;

+    /* replay counts */
+    uint256 constant RECURRING_SWAP_REPLAY_COUNT = 500;


I tend to think 500 is pretty high. We should just discuss as a product question generally

Should be easy to change this when we want to.

hayesgm · 2024-09-23T23:52:57Z

src/builder/Actions.sol

+        // operations, the `nonce` will be the `nonceSecret` (the hash chain has a length of 1)
+        bytes32 nonceSecret;
+        // The number of times an operation can be replayed. For non-replayable operations, this will be 0
+        uint256 replayCount;


On the backend, we've been more keen to use "totalPlays" which tends to map a little better since it doesn't have a discontinuity at 0.

Adjusted to totalPlays to be consistent. Both are fine imo.

hayesgm · 2024-09-23T23:53:44Z

src/builder/Actions.sol

@@ -245,6 +256,11 @@ library Actions {
        address paymentToken;
        string paymentTokenSymbol;
        uint256 paymentMaxCost;
+        // The secret used to generate the hash chain for a replayable operation. For non-replayable


Should we return nonce and nonceSecret? Generally we've been on "verify" mode on the client (as opposed to "generate"). That is, we run a check that hash_n(input.nonceSecret, input.totalPlays) == input.nonce

The actual nonce is on the QuarkOperation, so I don't think we need to put it here.

hayesgm · 2024-09-23T23:56:58Z

src/builder/Actions.sol

@@ -1487,6 +1544,18 @@ library Actions {
        return result;
    }

+    function generateNonceFromSecret(bytes32 secret, uint256 replayCount) internal pure returns (bytes32) {
+        assembly ("memory-safe") {


hayesgm · 2024-09-23T23:57:55Z

test/builder/lib/ReplayableHelper.sol

+pragma solidity ^0.8.27;
+
+library ReplayableHelper {
+    function generateNonceFromSecret(bytes32 secret, uint256 replayCount) internal pure returns (bytes32) {


Any reason not to use the optimized version above?

Mainly just because we don't need the optimized version here to save us from stack too deep. Also I thought it's kinda nice to have a more readable version so we can sanity check the optimized version in our tests with a different implementation.

hayesgm · 2024-09-24T23:08:22Z

src/builder/Accounts.sol

@@ -182,16 +181,17 @@ library Accounts {
        return findAssetPositions(assetAddress, chainAccounts.assetPositionsList);
    }

-    function findQuarkState(address account, Accounts.QuarkState[] memory quarkStates)
+    function findQuarkSecret(address account, Accounts.QuarkSecret[] memory quarkSecrets)


TBF, if we needed to do two operations on the same chain, this strategy wouldn't work? E.g. let's say we wanted to make two delayed operations on the same chain.

Adds `nonceSecret` and `replayCount` to QuarkBuilder. For replayable operations, the QuarkBuilder will also generate the nonce using the `nonceSecret` and `replayCount` (see `generateNonceFromSecret` function). We also uncover and fix a bug where `0` is being used as the `nonce` for an operation when a nonce is not provided for an account by the client. Now, the code reverts if a `nonceSecret` is not given for an account. We buff up the unit tests to assert the right values are being used for the nonces, since nonces were being checked previously.

Support new Quark v2 interfaces in QuarkBuilder

17b2317

kevincheng96 changed the title ~~Support new Quark v2 interfaces in QuarkBuilder~~ Support Quark v2 in QuarkBuilder Sep 23, 2024

kevincheng96 added 2 commits September 23, 2024 13:50

Fix stack too deep using assembly

7c0d151

More stack too deep fixes

92707c2

kevincheng96 requested review from hayesgm and fluffywaffles September 23, 2024 22:44

kevincheng96 commented Sep 23, 2024

View reviewed changes

hayesgm reviewed Sep 23, 2024

View reviewed changes

Change replayCount to totalPlays

8e995e6

hayesgm approved these changes Sep 24, 2024

View reviewed changes

kevincheng96 merged commit bf5b1aa into kevin/quark-v2 Sep 24, 2024
4 checks passed

kevincheng96 deleted the kevin/builder-v2 branch September 24, 2024 23:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Quark v2 in QuarkBuilder #83

Support Quark v2 in QuarkBuilder #83

kevincheng96 commented Sep 23, 2024 •

edited

Loading

kevincheng96 Sep 23, 2024

hayesgm Sep 23, 2024

kevincheng96 Sep 24, 2024 •

edited

Loading

hayesgm Sep 24, 2024

kevincheng96 Sep 24, 2024 •

edited

Loading

hayesgm Sep 23, 2024

kevincheng96 Sep 24, 2024

hayesgm Sep 23, 2024

kevincheng96 Sep 24, 2024

hayesgm Sep 23, 2024

kevincheng96 Sep 24, 2024

hayesgm Sep 23, 2024

hayesgm Sep 23, 2024

kevincheng96 Sep 24, 2024

hayesgm Sep 24, 2024

Support Quark v2 in QuarkBuilder #83

Support Quark v2 in QuarkBuilder #83

Conversation

kevincheng96 commented Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevincheng96 Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevincheng96 Sep 24, 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

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

kevincheng96 commented Sep 23, 2024 •

edited

Loading

kevincheng96 Sep 24, 2024 •

edited

Loading

kevincheng96 Sep 24, 2024 •

edited

Loading