-
Notifications
You must be signed in to change notification settings - Fork 4
fix: add missing check for sequence numbers in commit report offramp #460
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
Conversation
… vv/fix-offramp-commit-sequence-nr
|
👋 vicentevieytes, 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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds validation to prevent invalid sequence number intervals in commit reports. The title indicates a fix for missing checks on sequence numbers in the offramp commit report.
- Adds validation to ensure
minSeqNrmatches the source chain config andminSeqNr <= maxSeqNr - Implements previously stubbed
loadmethods for deserializing commit report data structures - Adds comprehensive test coverage for the new validation rules
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/ccip/bindings/offramp/offramp.go | Adds ErrorInvalidInterval error constant |
| contracts/wrappers/ccip/OffRamp.ts | Adds InvalidInterval error enum value and implements load methods for RampMessageHeader, Any2TVMRampMessage, ExecutionReport, and execute message |
| contracts/tests/ccip/OffRamp.spec.ts | Adds two test cases validating the new sequence number interval checks |
| contracts/contracts/ccip/offramp/errors.tolk | Adds InvalidInterval error to the error enum |
| contracts/contracts/ccip/offramp/contract.tolk | Adds assertion to validate sequence number intervals in commit reports |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
patricios-space
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we bundle this fix with the ones we are merging in PR #430?
Co-authored-by: Patricio <contact@patricios.space>
If you think that's better we can do that. I don't think it's necessary. |
patricios-space
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome
No description provided.