Skip to content
This repository has been archived by the owner on Sep 23, 2024. It is now read-only.

Adding authorized proof signers mapping #110

Merged
merged 1 commit into from
Mar 30, 2024

Conversation

praetoriansentry
Copy link
Contributor

@praetoriansentry praetoriansentry commented Mar 29, 2024

The current implementation of the AggLayer service requires that the party submitting the proof sign the request with the trusted sequencer's key. This makes sense given that we can easily verify the trusted sequencer from the rollup contract.

This PR adds an alternative configuration which would allow for a proof to be submitted using a configurable key rather than the sequencer key. This should allow for better isolation of key material because the sequencer key will not need to be used by multiple roles.

Part of the aim here is to be minimally invasive in this changeset. We're not changing the beneficiary logic at all and specifically allowing for an alternative signer.

Most of the testing for this takes place in 0xPolygon/kurtosis-cdk#38 allowed me to try a few cases:

  • Aggregator using the sequencer key to sign
  • Aggregator using a proof signer to sign
  • Aggregator sequencer to sign but the full [ProofSigners] section is missing
  • Test and Verification on Cardona

As an example this is what the config would look like:

[FullNodeRPCs]
1 = "http://zkevm-node-rpc-001:8123"

[ProofSigners]
1 = "0x7569cc70950726784c8D3bB256F48e43259Cb445"

@praetoriansentry praetoriansentry changed the title Jhilliard/authorized signers Adding authorized proof signers mapping Mar 29, 2024
@praetoriansentry praetoriansentry marked this pull request as ready for review March 29, 2024 22:01
@praetoriansentry praetoriansentry requested a review from a team as a code owner March 29, 2024 22:01
Copy link
Contributor

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

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

LGTM but missing a default value in config/defaults.go, it can be empty but needs to be there.

Copy link
Contributor

@zuiris zuiris left a comment

Choose a reason for hiding this comment

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

LGTM

interop/executor.go Outdated Show resolved Hide resolved
Copy link
Contributor

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

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

LGTM

@vcastellm vcastellm force-pushed the jhilliard/authorized-signers branch 3 times, most recently from 9909751 to 47ab08c Compare March 30, 2024 11:51
* Test arbitrary configured signer
* Modify the workflow so we can omit the request when there's a locally configured signer
@vcastellm vcastellm force-pushed the jhilliard/authorized-signers branch from 47ab08c to af20f1f Compare March 30, 2024 11:57
Copy link

sonarcloud bot commented Mar 30, 2024

@vcastellm vcastellm merged commit f311637 into main Mar 30, 2024
6 checks passed
@vcastellm vcastellm deleted the jhilliard/authorized-signers branch March 30, 2024 12:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants