-
Notifications
You must be signed in to change notification settings - Fork 2
validate only one ramp message per execute report #304
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
validate only one ramp message per execute report #304
Conversation
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 refactors the TON (The Open Network) execute codec to enforce a single message per execute report constraint. The changes ensure that the codec properly validates and processes only one ramp message per execution report, aligning with TON's architectural limitations.
- Adds validation to ensure exactly one message is present in each chain report
- Simplifies the code by removing the loop that processed multiple messages and directly handles the single message
- Updates test configuration to generate reports with only one message per report
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/ccip/codec/executecodec.go | Adds validation for single message constraint and refactors encoding logic to handle one message directly |
| pkg/ccip/codec/executecodec_test.go | Updates test configuration to generate only one message per report |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }) | ||
| } | ||
|
|
||
| // TODO, re-enable when gas limit from extra args is supported |
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.
what's pending to add the gas limit? @huangzhen1997 @vicentevieytes
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.
There were commented out because we skipped gasLimit from the contract. But Vicente is adding them back on-chain, so we are going to uncomment this soon in another PR
|
@ogtownsend please review |
pkg/ccip/codec/executecodec.go
Outdated
| executeReport := ocr.ExecuteReport{ | ||
| SourceChainSelector: uint64(chainReport.SourceChainSelector), | ||
| Messages: rampMessages[0], | ||
| Messages: rampMessage, |
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.
I think the field name Messages can be singular. it's a bit misleading(same on the contract)
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.
Renamed plugin. Don't see where contract uses plural
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.
In offramp, but I'd suggest fixing this in contract PRs since there are many things going on
cc. @patricios-space @vicentevieytes
| assert(!report.messages.beginParse().isEmpty(), Error.EmptyExecutionReport); |
| messages: cell; // vec<Any2TVMRampMessage> |
Co-authored-by: Copilot <[email protected]>
Jira : https://smartcontract-it.atlassian.net/browse/NONEVM-2927?atlOrigin=eyJpIjoiOTg3Mjc5Mzk2MzBkNDQ2NDlkZjRmMTM3NmE1MjExNWIiLCJwIjoiaiJ9