Update signature verification for CommitteeVerifier and CRE Forwarder contracts#137
Open
faisal-chainlink wants to merge 20 commits into
Open
Update signature verification for CommitteeVerifier and CRE Forwarder contracts#137faisal-chainlink wants to merge 20 commits into
faisal-chainlink wants to merge 20 commits into
Conversation
Soroban Contract Test Coverage92.66% line coverage — 17891 / 19308 lines hit
Per-Contract Breakdown
Full file-level coverage report |
faisal-chainlink
commented
Jul 2, 2026
| const SECP256K1_ORDER: [u8; 32] = [ | ||
| 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xfe, | ||
| 0xba, 0xae, 0xdc, 0xe6, 0xaf, 0x48, 0xa0, 0x3b, 0xbf, 0xd2, 0x5e, 0x8c, 0xd0, 0x36, 0x41, 0x41, | ||
| ]; |
Collaborator
Author
There was a problem hiding this comment.
Removed because Soroban's crypto SDK applies this check automatically for ECDSA and it is not needed for ED25519 (no recovery).
faisal-chainlink
commented
Jul 2, 2026
faisal-chainlink
requested review from
FelixFan1992 and
ilija42
and removed request for
ilija42
Integration Test Coverage (excl. Token Pool) |
Integration Test Coverage (Token Pool) |
faisal-chainlink
changed the title
Collaborator
Author
|
The E2E tests are expected to fail here due to changes in |
ilija42
reviewed
Jul 10, 2026
ilija42
left a comment
ilija42
left a comment
Collaborator
There was a problem hiding this comment.
The contract changes look fine to me
| const SECP256K1_ORDER: [u8; 32] = [ | ||
| 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xfe, | ||
| 0xba, 0xae, 0xdc, 0xe6, 0xaf, 0x48, 0xa0, 0x3b, 0xbf, 0xd2, 0x5e, 0x8c, 0xd0, 0x36, 0x41, 0x41, | ||
| ]; | ||
|
|
||
| // Storage TTL constants (ledger counts; 1 ledger ≈ 5 s on Mainnet). | ||
| // TODO adjust |
Collaborator
Author
There was a problem hiding this comment.
I've removed the TODO but it's upto you if you'd like to adjust the values.
| @@ -174,7 +193,7 @@ impl KeystoneForwarder { | |||
| receiver: Address, | |||
| raw_report: Bytes, | |||
| report_context: Bytes, | |||
| signatures: Vec<BytesN<65>>, | |||
| signatures: Vec<Ed25519Signature>, | |||
Collaborator
There was a problem hiding this comment.
I thought that we could support both ed and ecdsa signatures?
Collaborator
Author
There was a problem hiding this comment.
we can but I want to clarify, do you want to support both at the same time? or have a utility that would allow switching from one to the other? The latter version is already in place.
Collaborator
Author
There was a problem hiding this comment.
The added SignatureScheme trait has 2 implementing structs Ed25519 and Secp256k1EthAddress. Either one could be used. The method they expose is identify_signer which panics if the signature is invalid (these are in contracts/common/signature/src/scheme.rs).
Collaborator
Author
There was a problem hiding this comment.
Note that if you want to support both at the same time, we'll need to store them in 2 different storage keys since these signature types require different inputs. For example, in the case of Ed25519, it is impossible to recover the public key from just the signature.
Collaborator
There was a problem hiding this comment.
we can but I want to clarify, do you want to support both at the same time? or have a utility that would allow switching from one to the other? The latter version is already in place.
Not sure if there is much value in switching, but if we can support both, then in theory you could have a workflow that generates one report that can be sent to multiple chains
| Ok(()) | ||
| } | ||
| } | ||
|
|
||
| #[contractimpl] | ||
| impl KeystoneForwarder { | ||
| pub fn initialize(env: Env, owner: Address) -> Result<(), ForwarderError> { |
Collaborator
There was a problem hiding this comment.
I think I copied this over from the CCIP contract, but I wonder if we should set the owner at the deployment time
… CommitteeVerifier contracts
… sig quorum trait
ilija42
force-pushed
the
feat/signature-verification-refactor
branch
from
76da5cf to
21ff883
Compare
|
Code coverage report:
|
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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 PR applies the following changes to
contracts/ccvs/committee-verifierandcontracts/cre:contracts/common/signature(renamed fromcontracts/common/verifier) to include basic traits around signature config storage and validation for both ECDSA and ED25519.SignatureConfigManagerto handle signature verification storage (both instance and persistent) with an associated typeDataKeyfor a data access (defined by the concrete contracts)#[contracttrait]from the signature verification traits to allow using generics in traits. This means all methods defined in the traits mentioned will be internal to the contract and not exposed by default. Any method (ex:#KeystoneForwarder.set_configmust be defined in the contract itself).SignatureSchemetrait with 2 implementing structs (Secp256k1EthAddressandEd25519) to serve as default implementations / helpers.SignatureQuoruma bit to remove any dependency on the actual implementation of CommitteeVerifier. This currently only works forSecp256k1EthAddressbut can be easily extended to use the existingEd25519as well. The primary difference in usage between CRE and CCV for quorum is not the signature itself (can be easily differentiated with the changes in this PR) but rather than way quorum is determined. A follow up PR would allow for branching based on quorum mode (SignatureVerificationConfig::FailurevsSignatureVerificationConfig::Threshold).