[ReceiveExecutor] ref: store data in state machine - #826
Conversation
65ec078 to
4ff9547
Compare
There was a problem hiding this comment.
Pull request overview
Refactors the CCIP ReceiveExecutor contract/state layout to persist token-transfer execution details inside a dedicated state machine, shifting token-transfer initiation to be driven by InitExecute message contents rather than standalone storage fields.
Changes:
- Replaces
ReceiveExecutor_MessageStatewithReceiveExecutor_State { tokenTransfer?, messageExecution }and introduces a taggedReceiveExecutor_TokenTransferStateunion for token-transfer flow. - Extends
ReceiveExecutor_InitExecuteto optionally carrytokenAdminRegistry, and updatesOffRampto populate this field when available. - Restructures ReceiveExecutor tests by extracting shared setup utilities and adding a dedicated execution/token-transfer test suite.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| contracts/wrappers/gen/ccip/ReceiveExecutor.ts | Updates generated wrapper types/serialization for new storage + state machine model and InitExecute payload changes. |
| contracts/wrappers/gen/ccip/OffRamp.ts | Updates generated wrapper structs/serialization for the extended ReceiveExecutor_InitExecute. |
| contracts/tests/ccip/receiveExecutor/ReceiveExecutor.spec.ts | Simplifies the core ReceiveExecutor test suite and delegates deployment to shared setup. |
| contracts/tests/ccip/receiveExecutor/ReceiveExecutor.Setup.ts | Adds reusable test deployment + message factory helpers for ReceiveExecutor. |
| contracts/tests/ccip/receiveExecutor/ReceiveExecutor.execution.spec.ts | Adds execution-flow and token-transfer-flow tests, including retry behaviors. |
| contracts/contracts/lib/utils.tolk | Adds Cell<T>?.loadOpt() helper for ergonomic optional cell loading. |
| contracts/contracts/ccip/receive_executor/types.tolk | Defines the new ReceiveExecutor_State and ReceiveExecutor_TokenTransferState union and helpers. |
| contracts/contracts/ccip/receive_executor/storage.tolk | Updates storage layout to use the new state struct and removes old token-transfer fields. |
| contracts/contracts/ccip/receive_executor/messages.tolk | Extends ReceiveExecutor_InitExecute with optional tokenAdminRegistry. |
| contracts/contracts/ccip/receive_executor/contract.tolk | Refactors message handlers to use the new state machine and validates sender based on persisted token-transfer state. |
| contracts/contracts/ccip/offramp/contract.tolk | Populates initExecuteMsg.tokenAdminRegistry and removes embedding token-transfer fields into ReceiveExecutor storage init. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
4ff9547 to
a82a767
Compare
a82a767 to
9ad430c
Compare
9ad430c to
4adfc5b
Compare
4adfc5b to
7f2fd20
Compare
| struct ReceiveExecutor_TokenTransferState_Untouched { | ||
| tokenAdminRegistry: address; | ||
| } | ||
|
|
||
| struct ReceiveExecutor_TokenTransferState_TokenAdminRegistryQuery { | ||
| tokenAdminRegistry: address; | ||
| } | ||
|
|
||
| struct ReceiveExecutor_TokenTransferState_TokenAdminRegistryQueryFailed { | ||
| tokenAdminRegistry: address; | ||
| } | ||
|
|
||
| struct ReceiveExecutor_TokenTransferState_ReleaseOrMint { | ||
| tokenAdminRegistry: address; | ||
| tokenPool: address; | ||
| } | ||
|
|
||
| struct ReceiveExecutor_TokenTransferState_ReleaseOrMintFailed { | ||
| tokenAdminRegistry: address; | ||
| tokenPool: address; | ||
| } |
There was a problem hiding this comment.
I'm not sure that i see the advantage of having all these structs which are basically the same as different types over having an enum that indicates what the state is and a single storage type.
That said I'm okay with it if you feel like it improves the code.
There was a problem hiding this comment.
Something like this seems easier to follow to me
struct ReceiveExecutor_State {
tokenTransfer: Cell<ReceiveExecutor_TokenTransfer>?
messageExecution: ReceiveExecutor_MessageExecutionState
}
struct ReceiveExecutor_TokenTransfer {
state: ReceiveExecutor_TokenTransferState
tokenAdminRegistry: address;
tokenPool: address;
}
enum ReceiveExecutor_TokenTransferState {
Untouched
TokenAdminRegistryQuery
TokenAdminRegistryQueryFailed
ReleaseOrMint
ReleaseOrMintFailed
}
and if tokenTransfer is null then there is no tokenTransfer and that's it
There was a problem hiding this comment.
The issue with this is that the logic is not represented by the types. Let's take the tokenPool field, for example. It should be an optional in your model, as it is not populated until You passed the token registry query.
- Let's say your release or mint failed, so
ReceiveExecutor_TokenTransfer.state == ReceiveExecutor_TokenTransferState.ReleaseOrMintFailed. - You receive an initExec message from a manual exec.
- You match your state and know you have to retry the ReleaseOrMint, so you try to use
ReceiveExecutor_TokenTransfer.tokenPoolbut it is an optional. As it is an optional, you would need to put anassert (ReceiveExecutor_TokenTransfer.tokenPool != null) throw Unreachable, which means you have lost information. Your type system doesn't keep track of the relation between your knowleged of the TokenPool address and your stateReleaseOrMintFailedwhich entails the fact that you have already succeeded quering the token registry.
There was a problem hiding this comment.
I've reconsider it and I think you got a point. Please take a look at the latest version. I've extracted the tokenAdminRegistry, which was shared across all states, and kept only the tokenPool state dependant
parse, don't validate