[CRE-4317] Implement EVM ocr3types.OnchainKeyring2 - #469
Conversation
✅ API Diff Results -
|
There was a problem hiding this comment.
Pull request overview
Adds an EVM implementation of ocr3types.OnchainKeyring2 backed by chainlink-common/keystore, including signature compression compatible with the OCR3 ECDSA attestation verifier, plus test vectors and dependency bumps required for the new interfaces.
Changes:
- Implement
CreateOCR3OnchainKeyring/ListOCR3OnchainKeyringsand an EVM-backedOnchainKeyring2implementation. - Add OCR3 onchain signing/verification test vectors to validate hashing + signature compression behavior.
- Bump
chainlink-common/keystore,libocr, and other indirect deps ingo.mod/go.sum.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| pkg/keys/v2/ocr3_onchain_keyring.go | New OCR3 onchain keyring implementation (hashing + signature compression). |
| pkg/keys/v2/ocr3_onchain_keyring_test.go | New test-vector based coverage for OCR3 onchain signing/verification. |
| go.mod | Dependency updates to pull in required OCR3/keystore interfaces. |
| go.sum | Checksums updated for bumped dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // The generic RI parameter is not used anywhere in the implementation. It acts as a defensive interface boundary to | ||
| // avoid signing a wrong type of reports. Note that it is not 100% bulletproof since one can also create a key with | ||
| // CreateOCR3OnchainKeyring[A] and then read it with ListOCR3OnchainKeyrings[B]. | ||
| func CreateOCR3OnchainKeyring[RI any](ctx context.Context, ks keystore.Keystore, keyringName string) (ocr3types.OnchainKeyring2[RI], error) { |
There was a problem hiding this comment.
@pavel-raykov , do we still want RI here? My understanding of our previous discussion was that we wanted consumers of the library to follow this pattern:
keyring := keys.CreateOCR3OnchainKeyring(ctx, ks, "test-key")
ocr3keyring := ocr3util.AsOCR3OnchainKeyring2IgnoringRI[whatver](keyring)
// pass ocr3keyring to libocr via OCR3OracleArgs2
There was a problem hiding this comment.
Yes, RI is a part of the interface coming from OCR library, so the clients should have a convenient function to instantiate it (by itself RI-less interface should be useless to them)
There was a problem hiding this comment.
Updated the function name with IgnoringRI suffix and updated the comment
| // Be careful! The generic RI parameter is not used anywhere in the implementation. It acts as a defensive interface boundary to avoid signing a wrong type of reports. | ||
| // If you have security-critical information in RI, you should make your own ocr3types.OnchainKeyring2 that does use the information in RI for signing. | ||
| func CreateOCR3OnchainKeyringIgnoringRI[RI any](ctx context.Context, ks keystore.Keystore, keyringName string) (ocr3types.OnchainKeyring2[RI], error) { |
PhilippSchindler
left a comment
PhilippSchindler
left a comment
There was a problem hiding this comment.
I've reviewed the cryptographic part of the implementation.
Overall verdict: Looks good — the implementation is correct and closely matches what we previously implemented in offchain-reporting (PR #528). The (r, s) v-stripping scheme, the s to n-s normalization for v=0, the report hashing (incl. comment regarding domain separation), and Verify all check out.
One minor suggestion I had to remove a latent footgun (uncompressSignature) was already addressed.
LGTM, great. 👍
Requires smartcontractkit/chainlink-common#2137