Skip to content

Quark v2 changes#81

Merged
kevincheng96 merged 8 commits intomainfrom
kevin/quark-v2
Sep 30, 2024
Merged

Quark v2 changes#81
kevincheng96 merged 8 commits intomainfrom
kevin/quark-v2

Conversation

@kevincheng96
Copy link
Copy Markdown
Contributor

@kevincheng96 kevincheng96 commented Sep 16, 2024

Updating the quark-scripts repo to depend on the latest version of Quark (Quark v2). The main changes in this PR are:

  • Update quark submodule to the latest commit on the Quark v2 branch
  • Bump up Solidity version to 0.8.27 and EVM version to cancun
  • Update QuarkOperation struct to the newest version that has a isReplayable field
  • Update RecurringSwap.sol to no longer need a cancel function and no longer need to call allowReplay()
  • Bring in the Cancel.sol core script, used for cancelling replayable operations
  • Update QuarkBuilder to use Quark v2; add the new fields nonceSecret and totalPlays to QuarkBuilder
  • Optimize QuarkBuilder to get rid of stack too deep errors from introducing the new fields

@kevincheng96 kevincheng96 force-pushed the kevin/quark-v2 branch 6 times, most recently from 49bd101 to 2b91662 Compare September 16, 2024 22:54
@kevincheng96 kevincheng96 changed the title Kevin/quark v2 Quark v2 changes Sep 16, 2024
@kevincheng96 kevincheng96 marked this pull request as ready for review September 16, 2024 23:32
Comment thread src/builder/Actions.sol Outdated
// Construct QuarkOperation
IQuarkWallet.QuarkOperation memory quarkOperation = IQuarkWallet.QuarkOperation({
nonce: accountState.quarkNextNonce,
nonce: bytes32(uint256(accountState.quarkNextNonce)),
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a bunch of interface changes that need to be made in QuarkBuilder for the client to pass in a random nonce. I plan to do that as a separate PR.

Copy link
Copy Markdown
Contributor

@hayesgm hayesgm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm


return IQuarkWallet.QuarkOperation({
nonce: operation.nonce,
isReplayable: operation.isReplayable,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is interesting, and I think correct. The Paycall operation is repalyable IFF what it wraps is replayable?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly. If it's a RecurringSwap using Paycall, then the Paycall itself has to be replayable.

IQuarkWallet.QuarkOperation memory lastQuarkOperation = quarkOperations[quarkOperations.length - 1];
IQuarkWallet.QuarkOperation memory mergedQuarkOperation = IQuarkWallet.QuarkOperation({
nonce: lastQuarkOperation.nonce,
isReplayable: lastQuarkOperation.isReplayable,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, so basically if it's wrap and replayableSupply this is now replayableWrapAndSupply?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, if the last operation is replayable, the merged operation should be replayable.

kevincheng96 and others added 7 commits September 25, 2024 14:29
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.
This patch adds an EIP-712 change for Quark V2, which adds an
`isReplayable` field in the typehash of the EIP-712 struct.
@kevincheng96
Copy link
Copy Markdown
Contributor Author

kevincheng96 commented Sep 25, 2024

The latest commit (fc9d209) rebases on main and applies the Quark v2 changes to the newest MorphoClaimRewards action. After the rebase, we were hitting stack too deep errors again, so the commit also includes some optimizations to the QuarkBuilder code.

EDIT: Replaced that commit with e5e823c, which now just comments out the MorphoClaimRewards logic since it is currently unused. The root cause for the stack too deep errors is the yul optimizer. The optimizer will inline some of the internal function calls in the QuarkBuilder, causing stacking too deep issues. The way to fix this is to turn off the optimizer for the QuarkBuilder. However, since the Quark scripts should still be built with the optimizer on, there's not a good way to separate the compilation settings unless we split QuarkBuilder into its own foundry project.

@kevincheng96 kevincheng96 merged commit 3dba570 into main Sep 30, 2024
@kevincheng96 kevincheng96 deleted the kevin/quark-v2 branch September 30, 2024 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants