Skip to content
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

ED-4619: Apply L3 Proving changes for ROUTES-1-6-L3 #143

Closed
wants to merge 8 commits into from

Conversation

johnwhitton
Copy link
Contributor

No description provided.

@johnwhitton johnwhitton changed the base branch from main to ROUTES-1-5 January 8, 2025 16:23
@johnwhitton johnwhitton changed the base branch from ROUTES-1-5 to ROUTES-1-6-L3 January 8, 2025 16:45
@johnwhitton johnwhitton changed the title ED-4619: Apply L3 Proving changes for Routes 1.5 ED-4619: Apply L3 Proving changes for ROUTES-1-6-L3 Jan 8, 2025
@johnwhitton johnwhitton marked this pull request as ready for review January 8, 2025 22:06
@johnwhitton johnwhitton requested review from nknavkal and re1ro January 8, 2025 22:06
@@ -1,6 +1,6 @@
/* -*- c-basic-offset: 4 -*- */
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.26;
pragma solidity ^0.8.28;
Copy link
Collaborator

Choose a reason for hiding this comment

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

not relevant to your PR but i think we should combine IProver and ISimpleProver

// Output slot for the game status (aaaaa)
uint256 internal constant L2_FAULT_DISPUTE_GAME_STATUS_SLOT = 0;

struct FaultDisputeGameStatusSlotData {
Copy link
Collaborator

Choose a reason for hiding this comment

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

im overthinking this but how do you feel about the structs being in here? the pure functions for sure absolutely make sense, but the structs to me look a bit odd in a library. If you wanna check out what I did for the Intents type I think that makes the most sense, I stole it from uniswap - kinda makes a file of structs that can be imported individually. But also this is totally fine

}

proveStorageBytes32(
require(
Copy link
Collaborator

Choose a reason for hiding this comment

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

custom errors

Copy link
Collaborator

Choose a reason for hiding this comment

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

could put this in the prover library or even a more general utils library since its just about encoding

Copy link
Collaborator

@nknavkal nknavkal left a comment

Choose a reason for hiding this comment

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

decent amount of cleanup work, and some reorganization, but mostly looks good

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.

3 participants