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

Add support for deploying a mock light client contract #1322

Merged
merged 10 commits into from
Apr 17, 2024
Merged

Conversation

nomaxg
Copy link
Contributor

@nomaxg nomaxg commented Apr 11, 2024

  • Contract deployment script now supports a mock light client contract that has less ZK machinery
  • STAKE_TABLE_CAPACITY is now configurable (a lower capacity really speeds up integration tests/ demos).

@nomaxg nomaxg assigned alxiong and unassigned alxiong Apr 15, 2024
@nomaxg nomaxg requested a review from alxiong April 16, 2024 13:55
@nomaxg nomaxg merged commit bdb3e0c into main Apr 17, 2024
15 of 16 checks passed
@nomaxg nomaxg deleted the mock-light-client branch April 17, 2024 00:24
Comment on lines +76 to +77
//

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
//

:)

@@ -439,24 +448,26 @@ mod tests {
.map(|b| if b { F::from(1u64) } else { F::from(0u64) })
.collect::<Vec<_>>();
// good path
let (circuit, public_inputs) = build::<_, _, _, _, _, ST_CAPACITY>(
let (circuit, public_inputs) = build::<_, _, _, _, _>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let (circuit, public_inputs) = build::<_, _, _, _, _>(
let (circuit, public_inputs) = build(

one benefit of removing that const generic is to remove all the generic type hint.
same comment applied to everywhere else where we still have dangling <_, _, ...>

@@ -119,6 +124,7 @@ processes:
- ESPRESSO_SEQUENCER_PRIVATE_STAKING_KEY=$ESPRESSO_DEMO_SEQUENCER_STAKING_PRIVATE_KEY_1
- ESPRESSO_SEQUENCER_PRIVATE_STATE_KEY=$ESPRESSO_DEMO_SEQUENCER_STATE_PRIVATE_KEY_1
- ESPRESSO_SEQUENCER_ETH_ACCOUNT_INDEX=11
- ESPRESSO_SEQUENCER_STAKE_TABLE_CAPACITY=10
Copy link
Contributor

Choose a reason for hiding this comment

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

are there any way to define a global (to all sequencer docker) env var declaration, so that we can centrally manged,
in case we forget to update one of them in the future?

Comment on lines +259 to 268
let light_client = LightClientMock::new(
contracts
.deploy_fn(Contract::LightClient, |contracts| {
deploy_light_client_contract(l1.clone(), contracts).boxed()
deploy_light_client_contract(
l1.clone(),
contracts,
opt.use_mock_contract,
)
.boxed()
})
Copy link
Contributor

Choose a reason for hiding this comment

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

wait, shouldn't you gate this using use_mock_contract: bool ?
this code seems to always use LightClientMock in the main()?

@@ -313,7 +342,7 @@ async fn deploy_light_client_contract<M: Middleware + 'static>(
.context("error linking PlonkVerifier lib")?;
bytecode
.link_fully_qualified(
"contracts/src/libraries/LightClientStateUpdateVK.sol:LightClientStateUpdateVK",
"contracts/tests/mocks/LightClientStateUpdateVK.sol:LightClientStateUpdateVK",
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, a bit confused why are we using /mocks/ inside deploy_production_() function

@alxiong
Copy link
Contributor

alxiong commented Apr 18, 2024

don't worry about my comments above, there are some legacy complication of deploying LC and LCMock differently, since one of them is upgradable, the other is not. It's probably faster for me to work on it.

I'm gonna address them in a follow-up PR, building on your foundation.
@nomaxg

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.

4 participants