-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Quark v2 #216
Merged
Merged
Quark v2 #216
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As part of our redesign for Quark v2, we decided to scrap isolated storage to simplify the flows for executing a Quark operation and lower the gas overhead. By removing isolated storage, we can remove the callback (`setActiveNonceAndCallback`) from the `QuarkStateManager` to the `QuarkWallet`. We can also remove the codepaths related to `activeNonceScript`, `nonceScriptAddress`, and `walletStorage`. This leaves only the nonce tracking logic in the `QuarkStateManager`. We are also temporarily disabling replay functionality in this PR. Replays will be brought back in a follow-up PR that implements the new replay token mechanism. I've commented any tests that have to do with replays, leaving a TODO (`// TODO: Uncomment when replay tokens are supported`) to revisit once they are supported again. There are also some TODOs to switch from sstore/ssload to tstore/tload. To use tstore/tload, we need to bump up our version of Solidity to >0.8.24. I plan to do that in a separate PR.
This code leads out the core changes for replayables with token. This was meant to be a WIP that just showed off the core features (it still is), but to get it to compile, I ended up converting most of the current tests to the new framework (e.g. mostly converting them to pick a semi-random nonce versus `nextNonce`, and checking `getNonceToken` as opposed to `getNextNonce`). Overall, the changes to the non-test code are very straight-forward. Most notable should be the changes to `QuarkStateManager` (adding new nonce and replay token code), and adding a new function to QuarkWallet `verifySigAndExecuteReplayableQuarkOperation`. Note: I didn't spend much time working on the outer interface for this, and I need to consider how this works best with multi-quark operations, but I wanted to get the outline first so we could discuss. There are also failing test cases and absent test cases, but again, more of a discussion point than a final product here. --------- Co-authored-by: kevincheng96 <[email protected]>
This improves the consistency of events in `QuarkNonceManager`. A nonce could be canceled either via `cancel` or `submit`, and the two paths currently will emit different events. This change ensures that cancelling using either approach will always emit the same events.
fluffywaffles
approved these changes
Sep 17, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙌
hayesgm
reviewed
Sep 17, 2024
We need to set the callback slot to 0 before executing a script to ensure that nested operations don't inherit the callback value of the parent operation.
kevincheng96
added a commit
to compound-finance/quark-scripts
that referenced
this pull request
Sep 30, 2024
Updating the `quark-scripts` repo to depend on the latest version of Quark ([Quark v2](compound-finance/quark#216)). 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 --------- Co-authored-by: Geoff Hayes <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a moderately-sized redesign of Quark v1. The main goal of this redesign is to improve the security of replayable operations by making them only submittable by submitters that have access to a secret submission token. A secondary goal of this redesign is to simplify a bunch of the flows to reduce gas overhead and code complexity.
Changes:
submissionToken
. Previously, replayable operations were submittable by any addressQuarkStateManager
, which allows us to remove much of the complicated codepaths related tosetActiveNonceAndCallback
,activeNonceScript
,nonceScriptAddress
, andwalletStorage
QuarkStateManager
toQuarkNonceManager
since its job is now to only manage noncesQuarkOperation
struct to have aisReplayable
fieldcancun
TSTORE/TLOAD
to save gas