-
Notifications
You must be signed in to change notification settings - Fork 2
Execute report gobinding #51
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
pkg/ccip/bindings/types.go
Outdated
if err != nil { | ||
return nil, fmt.Errorf("failed to unpack inner array: %w", err) | ||
} | ||
if len(data) != int(length) { |
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.
Why do we store the length here, just to check for mismatch? This seems unnecessary
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.
We need to know where to stop reading for each array, and it's indicated by the length we stored before reading each array, when parsing the linked cell.
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 might be missing something, can you point me to the LOC where length is used to stop reading the current item?
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.
You are right the length is not used, I'm revisiting the implementation now. This is used for the offchainData for ccip types and in offramp 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.
There's a bug, I will fix it
Messages SnakeRef[Any2TONRampMessage] `tlb:"^"` | ||
OffChainTokenData SnakeBytes2D `tlb:"^"` | ||
Proofs SnakeData[Signature] `tlb:"^"` // []Signature |
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 kinda suspect we don't actually need three different ways to encode collections of data, do we? cc @archseer
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.
SnakeData stores array objects in a linked-cell format using the cell's data slice. It only supports structs that do not contain cell references, as it cannot be automatically serialized via TLB otherwise.
SnakeRef addresses this limitation by storing array objects in reference cells instead of the data slice, allowing support for structs that include cell references.
These three are different encoding schemes, but they provide optimal storage efficiency for complex data types.
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 was able to solve SnakeData and SnakeRef onchain when decoding in a single pass, but it was dependent on the typed Cell<T>
wrapper (Iterator parses directly, Iterator<Cell> loads a ref then parses it), but I'm not sure if there's a straightforward way to do that in Go.
I think we could at least reuse SnakeRef
for SnakeBytes2D
: it's essentially SnakeRef[SnakeData[[]byte]]`?
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 wasn't sure how the on-chain [][]byte was encoded, from previous conversation I thought we were going to SnakeBytes2D including array length. We can use SnakeRef[SnakeBytes], to make it simpler, it's just we will need to use more cell, as it produce fragmentation in the cell data.
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.
Just pushed a change
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
Implements Gobinding for CCIP ExecuteReport messages with new packing/unpacking utilities and tests, and updates dependencies.
- Introduce generic TLB packing/unpacking helpers (
packArrayWithStaticType
,SnakeData
, etc.) and comprehensive unit tests intypes.go
/types_test.go
. - Add
ExecuteReport
binding and corresponding encoding/decoding tests inexecutereport.go
/executereport_test.go
. - Remove old commit report implementation but left tests referencing
CommitReport
, causing a mismatch; update module dependencies and Nix vendor hash.
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
pkg/ccip/bindings/types.go | Add array/cell packing and unpacking functions (packArrayWithStaticType , pack2D... ). |
pkg/ccip/bindings/types_test.go | Add unit tests covering edge cases for all new TLB utilities. |
pkg/ccip/bindings/executereport.go | Define ExecuteReport and related TLB-annotated types. |
pkg/ccip/bindings/executereport_test.go | Add tests to validate encoding and decoding of ExecuteReport . |
pkg/ccip/bindings/commitreport.go | Removed commit report logic but tests still reference CommitReport (incomplete update). |
pkg/ccip/bindings/commitreport_test.go | Old tests pruned, but TestCommitReport_EncodingAndDecoding remains without implementation |
go.mod | Add Solana and Testify dependencies and bump versions. |
cmd/chainlink-ton/default.nix | Update vendorHash after dependency changes. |
Comments suppressed due to low confidence (1)
pkg/ccip/bindings/commitreport.go:3
- The
CommitReport
struct and TLB imports (fmt
,github.com/xssnick/tonutils-go/tlb
,tvm/cell
) were removed, but tests still referenceCommitReport
. Reintroduce the struct definition and necessary imports to restore compilation.
import (
pkg/ccip/bindings/types.go
Outdated
|
||
// Signature represents an ED25519 signature. | ||
type Signature struct { | ||
Sig []byte `tlb:"bits 512"` |
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.
Note: this now has a third field labeled pubkey that's the signer's public key
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.
// Signature represents an ED25519 signature. | ||
type Signature struct { | ||
Sig []byte `tlb:"bits 512"` | ||
Sig []byte `tlb:"bits 256"` |
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.
This is wrong, the signature should be 512 bits (64 bytes), plus the public key (32 bytes)
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 got confused when I review the contract code, you used vec
// Any2TONRampMessage represents ramp message, which is part of the execute report. | ||
type Any2TONRampMessage struct { | ||
Header RampMessageHeader `tlb:"."` | ||
Sender SnakeBytes `tlb:"^"` |
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 added a new type for this called CrossChainAddress, it's 1 byte length len
, followed by len
bytes (one cell, no snake data, max length of 64 bytes)
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.
It can either be in place or as ^
ref
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 don't see it in the main branch, is it not merged ?
Close this as changes are migrated to another PR #68 |
Gobinding for encoding/decoding execute report, following format from contract skeleton PR.