[NONEVM-5645] Migrate all contracts to abigen bindings - #808
[NONEVM-5645] Migrate all contracts to abigen bindings#808patricios-space wants to merge 14 commits into
Conversation
77afe3c to
86e6726
Compare
caefac6 to
b726c12
Compare
b726c12 to
183a399
Compare
305fe11 to
64f5b3c
Compare
| value: 0, | ||
| dest: userAddress, | ||
| body: OnRamp_MessageValidated<RemainingBitsAndRefs> { | ||
| body: OnRamp_MessageValidated_FromOnRamp { |
There was a problem hiding this comment.
hmm, why this _FromOnRamp suffix?
| // --- Outgoing messages --- | ||
|
|
||
| // So that ABIgen can generate codec for OnRamp_MessageValidated<RemainingBitsAndRefs> | ||
| type OnRamp_MessageValidated_FromOnRamp = OnRamp_MessageValidated<RemainingBitsAndRefs> |
There was a problem hiding this comment.
Consider renaming as OnRamp_MessageValidated_Any
| @@ -0,0 +1,46 @@ | |||
| import * as morph from 'ts-morph' | |||
There was a problem hiding this comment.
Executive Summary
This PR migrates all CCIP TON contract wrappers from hand-written TypeScript to auto-generated abigen bindings produced by the acton Tolk toolchain, post-processed via a ts-morph-based transform pipeline. This is a significant architectural change that replaces ~7.8k lines of manual wrapper code with generated equivalents plus ~400 lines of ts-morph transform logic. The migration covers Router, OnRamp, OffRamp, FeeQuoter, SendExecutor, ReceiveExecutor, MerkleRoot, Token Pools (BurnMint, LockRelease, JettonLockBox), MCMS/Timelock, and supporting types.
Verdict: The approach is sound and the migration is comprehensive, but there are several security and stability concerns with the ts-morph post-processing pipeline that should be addressed before merge.
Architecture Overview
Pipeline Flow
Acton.toml → acton wrapper --ts → raw .ts files → ts-morph transforms → final wrappers/gen/
Key Components
contracts/scripts/abigen.ts— Entry point: reads manifest, invokesacton wrapper --tsper contract, feeds output through transform pipelinecontracts/scripts/abigen/toml.ts— Minimal TOML parser for Acton.toml manifestscontracts/scripts/abigen/transforms.ts— ts-morph AST transforms (SnakedCell, CellRef, QueryID, Dict→Map, Error sorting)contracts/wrappers/gen/**— Generated output (not committed to git ideally, but committed here)contracts/wrappers/ccip/*.ts— Hand-written "manual" wrappers (reduced to opcodes/constants only)contracts/wrappers/gen/index.ts— Setup hooks for custom serializers (CrossChainAddress codec)
Security Analysis
🔴 HIGH: Custom Serializer Registry is Module-Level Mutable State
Every generated file has:
let customSerializersRegistry: Map<string, [CustomPackToBuilderFn<any> | null, CustomUnpackFromSliceFn<any> | null]> = new Map;Each module gets its own registry, which is not shared across files. The setupGenBindings() function in gen/index.ts calls Router.registerCustomPackUnpack(...), OnRamp.registerCustomPackUnpack(...), etc. — one per module.
Risk: If a test or deployment script forgets to call setupGenBindings() (or calls it but misses a module), ensureCustomSerializerRegistered will throw at runtime with a cryptic error. This is a silent configuration failure — no type-checking catches it.
Recommendation:
- Add a test that exercises the
ensureCustomSerializerRegisteredfailure path to ensure error messages are actionable - Consider making the registry initialization lazy with a clear "not initialized" sentinel state
- Document in a README that
setupGenBindings()MUST be called before any generated wrapper is used
🔴 HIGH: ts-morph AST Transforms Are Code-Generation-At-Build-Time
The entire type safety of the wrappers hinges on the correctness of the ts-morph transforms. If a transform mutates the wrong AST node, the generated code will silently produce incorrect serialization — potentially corrupting on-chain messages.
Key transforms of concern:
-
transformSnakedCell— Rewritestype SnakedCell<T> = c.Celltotype SnakedCell<T> = T[], then injectsstoreSnakedCellOf/loadSnakedCellOfhelpers. This is a semantic-level change to serialization behavior. If the AST matching logic (getObjectMethod, field detection) is off-by-one or matches the wrong node, it corrupts serialization. -
transformCellRef— Similar pattern: rewritesCellRef<T>toTwith implicit wrapping. ThetransformSnakedStackBoundaryfunction uses string matching on text content ('cell') to identify stack boundary objects — fragile. -
sortErrorsBlocks— UsesreplaceWithTextwith hand-constructed string interpolation. While low risk (only affects ordering), it demonstrates that the transforms operate at a text level within the AST, not purely semantically.
Recommendation:
- Add round-trip tests for each transform: generate a known input file, run the transform, and diff against an expected output. Commit these golden files.
- Consider adding
// GENERATED BY TRANSFORM: <transform_name>comments at the start of each mutated block for traceability - The
transformSnakedCelltransform modifies both type aliases AND inserts functions — ifSNAKED_HELPERSis inserted multiple times (e.g., if a source file is processed twice), it will produce duplicate function declarations
🟡 MEDIUM: Generated Files Use any Types
The custom serializer registry uses any:
let customSerializersRegistry: Map<string, [CustomPackToBuilderFn<any> | null, CustomUnpackFromSliceFn<any> | null]> = new Map;While this is pragmatic for generated code, it means type-checking won't catch mismatches between the registered callback signatures and the types they're invoked with.
Recommendation: Acceptable for generated code, but worth noting in documentation. The error messages from ensureCustomSerializerRegistered provide a safety net.
🟡 MEDIUM: TOML Parser is Ad-Hoc
contracts/scripts/abigen/toml.ts is a minimal TOML parser that handles only a subset. It's documented as such, which is good. However:
- It strips comments but doesn't handle inline comments within arrays
- It treats unrecognized values as raw strings (including potentially malicious values if the manifest is untrusted)
- No schema validation on parsed output — missing
output-dirthrows, but wrong types might silently pass
Recommendation: The manifest (Acton.toml) is an internal/trusted file, so this is low risk. The comment in the code ("it is not a general-purpose TOML implementation") is sufficient documentation.
🟢 LOW: Generated Code Disables ESLint
All generated files have /* eslint-disable */. This is standard practice for generated code. The transforms run on AST level, so linting would run after generation. Acceptable.
Stability Analysis
🔴 HIGH: Build Pipeline Fragility
The build pipeline has these tight coupling points:
acton wrapper --tsmust produce output compatible with the ts-morph transforms- If the
actontoolchain changes its output format, transforms will silently fail or produce incorrect code - The
useInMemoryFileSystem: truein ts-morph means file system state is never synced — if a transform crashes mid-way, the in-memory state is lost but disk writes still happen (the finalfs.writeFileSync)
Risk: A future acton upgrade could break all transforms without any test catching it (the transforms don't validate their own preconditions).
Recommendation:
- Pin
actonversion in the manifest or CI - Add integration tests that regenerate wrappers and compare against committed golden files
- Consider adding
--checkmode to abigen that validates transforms without writing output
🟡 MEDIUM: Transform Order Dependency
Transforms are applied sequentially:
sortErrorsBlocks(sourceFile)
transformSnakedCell(sourceFile)
transformCellRef(sourceFile)
transformQueryId(sourceFile)
transformDictionaryMaps(sourceFile)The order matters. If transformSnakedCell runs before transformCellRef, a field that is both SnakedCell<CellRef<T>> could be misprocessed. The code comments don't document the dependency order.
Recommendation: Document transform ordering in comments or add an assertion that detects if a transform was applied to already-transformed code.
🟡 MEDIUM: Test Migration Coverage
The PR modifies 98 files including ~50 test files. While all tests use the new bindings, there's a risk that some test assertions are now less precise because the generated bindings abstract more away.
Specifically:
contracts/tests/Logs.tswas reduced from 387 lines to a much smaller file — log assertion helpers may have lost edge casescontracts/tests/ExitCode.spec.tswas reduced by ~264 lines — exit code cross-validation between old and new modules is still present but condensed
Recommendation: Verify that the CI test suite runs with full coverage and that gas reports are within acceptable variance.
🟡 MEDIUM: Shell.nix source ~/.zshrc
shellHook = ''
source ~/.zshrc # <-- This is newThis is a developer convenience change but introduces non-determinism: different developers' ~/.zshrc will modify the shell environment differently.
Recommendation: Remove source ~/.zshrc from shell.nix — it should be committed separately or handled outside the repo. This is outside the scope of this PR and should be reverted.
Code Quality Observations
Positive
- Modular transform design — Each transform is isolated in its own function with clear pre/post conditions
- Comprehensive migration — All CCIP contracts, token pools, and governance (MCMS/Timelock) are covered
- Good separation of concerns — Hand-written wrappers now contain only constants/opcodes, while generated files handle all serialization
- Proper test migration — Tests use the new
wrappers/gen/imports consistently registerCustomPackUnpackpattern — Clean extensibility for custom types likeCrossChainAddress
Concerns
- No changelog or migration guide — Developers switching to the new bindings need to know about
setupGenBindings()and thewrappers/gen/import paths - Generated files are committed — This is debatable. Committing generated code is safe for reproducibility but means every
actonupgrade requires a PR with thousands of lines of generated changes contracts/wrappers/ccip/Logs.tsdeletion — The old Logs.ts contained hand-crafted log assertion logic. The newcontracts/tests/Logs.tsuses generated types but may have lost some of the nuanced matching logic
Specific PR Feedback (Publishable)
Required Before Merge
-
Remove
source ~/.zshrcfromshell.nix— This introduces non-deterministic developer environment behavior and should not be in the repo. -
Add golden-file tests for the abigen transform pipeline — Create a test that regenerates at least one wrapper (e.g.,
Router.ts) and diffs against a committed expected output. This ensures futureactonversion upgrades don't silently break the transforms. -
Document
setupGenBindings()requirement — Add aREADME.mdor comment incontracts/wrappers/gen/index.tsexplaining thatsetupGenBindings()must be called before any generated wrapper is used. -
Add a transform precondition check — Each transform should verify it's operating on untransformed
actonoutput (e.g., check thattype SnakedCell<T> = c.Cellexists before trying to transform it). If the pattern doesn't match, log a warning rather than silently skipping.
Nice to Have
-
Consider a
// GENERATEDheader — Add a comment at the top of each generated file indicating theactonversion and transform commit that produced it, for traceability. -
Add
.gitattributesfor generated files — Considerlinguist-generated=trueso Git ignores generated files in blame and contribution graphs. -
Gas report comparison — Include a diff of gas reports before/after this PR to confirm the generated wrappers produce equivalent code paths.
-
Transform ordering documentation — Add a comment in
abigen.tsdocumenting why transforms are applied in a specific order and what the dependency graph looks like.
Conclusion
This is a well-scoped and well-executed migration that significantly reduces the maintenance burden of hand-written TON contract wrappers. The ts-morph approach is the right architectural choice for post-processing acton output. However, the build-time code generation pipeline introduces new security and stability surface area that needs guarding with golden-file tests, transform precondition checks, and clear documentation of the setupGenBindings() requirement.
Recommendation: Request changes on the 4 required items above, then approve.
There was a problem hiding this comment.
Huge PR, nice work!
The bindings usage updates look good, but I do worry about the stability of the shim and how to support it. Also all code might get audited and looked at so it's additional cost to understand, support, and review for the team.
See above for potential improvements.
NONEVM-5645