Conversation
|
👋 yashnevatia, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
|
I see you updated files related to
|
|
✅ No conflicts with other open PRs targeting |
There was a problem hiding this comment.
Pull request overview
Risk Rating: MEDIUM — touches request hashing for write-report capabilities, which can affect request deduping/consensus behavior across nodes.
Adds Aptos support to the V2 WriteReportExcludeSignaturesHasher so Aptos WriteReportRequest messages are hashed consistently with EVM/Solana (signatures excluded).
Changes:
- Add Aptos capability protobuf import and hashing path for
aptos.WriteReportRequest. - Extend write-report family detection to recognize
CapabilityIdfamilyaptos. - Update the “unknown family” error message to include Aptos.
Comments suppressed due to low confidence (1)
core/capabilities/remote/executable/hasher.go:150
- The new Aptos write-report hashing branch isn’t covered by existing unit tests. Please add tests that build an Aptos WriteReportRequest payload and assert (1) same report bytes but different signatures produce the same hash and (2) nil Report errors, similar to the existing EVM/Solana coverage, to prevent regressions in signature-exclusion logic.
case writeReportFamilyAptos:
var wrReq aptoscappb.WriteReportRequest
if err = req.Payload.UnmarshalTo(&wrReq); err != nil {
return [32]byte{}, fmt.Errorf("failed to unmarshal Payload to WriteReportRequest: %w", err)
}
if wrReq.Report == nil {
return [32]byte{}, errors.New("WriteReportRequest.Report is nil")
}
wrReq.Report.Sigs = nil // exclude signatures from hash
payload, err = anypb.New(&wrReq)
if err != nil {
return [32]byte{}, fmt.Errorf("failed to marshal WriteReportRequest back to anypb: %w", err)
}
| return writeReportFamilyEVM, nil | ||
| case "solana": | ||
| return writeReportFamilySolana, nil | ||
| case "aptos": | ||
| return writeReportFamilyAptos, nil | ||
| } |
There was a problem hiding this comment.
In getWriteReportFamily, the parsing currently can’t fail in the intended way because strings.Split always returns at least one element. Consider removing the redundant length check above the switch and adding a meaningful validation for malformed capability IDs (e.g., empty family segment) so parsing errors are caught deterministically.
|
|
bolekk
left a comment
bolekk
left a comment
There was a problem hiding this comment.
LGTM. Could use a unit test.
|
Requires
Supports