-
Notifications
You must be signed in to change notification settings - Fork 2
Extend cldf timelock converter logic for TON #628
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
Extend cldf timelock converter logic for TON #628
Conversation
|
|
👋 huangzhen1997, 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 extends the CLDF (Chainlink Deployments Framework) proposal conversion logic to support the TON (The Open Network) blockchain. The changes enable TON chain integration by adding necessary converters, executors, and inspectors while also updating various dependencies.
Key changes:
- Add TON support to MCMS v2 CLI with timelock converter, executor, and inspector implementations
- Introduce
TxOpsstruct to encapsulate TON transaction operation configuration - Update multiple dependencies including Go version, chainlink-ccip, mcms, and tonutils-go
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| go.mod | Updates Go version to 1.25.3 and upgrades multiple dependencies including TON-related libraries |
| engine/cld/legacy/cli/mcmsv2/mcms_v2.go | Adds TON chain family support with converter, executor, and inspector implementations; refactors variable naming |
| chain/ton/ton_chain.go | Introduces TxOps struct to hold wallet and amount configuration for transactions |
| chain/ton/provider/rpc_provider.go | Initializes TxOps with default transaction amount in RPC provider |
| chain/ton/provider/ctf_provider_test.go | Updates test to verify TxOps instead of Wallet field |
| chain/ton/provider/ctf_provider.go | Initializes TxOps with default amount; fixes wallet method call to use WalletAddress() |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Client: nodeClient, | ||
| Wallet: tonWallet, | ||
| WalletAddress: tonWallet.Address(), | ||
| WalletAddress: tonWallet.WalletAddress(), |
Copilot
AI
Dec 12, 2025
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.
The change from tonWallet.Address() to tonWallet.WalletAddress() lacks test coverage. The existing test in ctf_provider_test.go only checks that WalletAddress is not empty but doesn't verify the correct method is being called or that the address value is as expected.
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.
@copilot This is not true, it's tested in rpc_provider_test.go line 276
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
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…rt-ton-timelock-converter
…lock-converter' of github.com:smartcontractkit/chainlink-deployments-framework into NONEVM-3069/extend-cldf-proposal-logic-support-ton-timelock-converter
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
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return nil, fmt.Errorf("error creating Sui timelock converter: %w", err) | ||
| } | ||
| case chainsel.FamilyTon: | ||
| converter = ton.NewTimelockConverter(tlb.MustFromTON(defaultTONExecutorAmount)) |
Copilot
AI
Jan 27, 2026
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.
The TON timelock converter initialization lacks test coverage. Consider adding a test case similar to the existing Sui test case at line 1136-1138 to verify the converter is created correctly for TON chains.
| Amount: tlb.MustFromTON(defaultTONExecutorAmount), | ||
| } | ||
|
|
||
| return ton.NewExecutor(opts) |
Copilot
AI
Jan 27, 2026
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.
The TON executor creation in getExecutorWithChainOverride lacks test coverage. Add test cases to verify the executor is created correctly with the expected options, including the Amount field set to defaultTONExecutorAmount.
| Amount: tlb.MustFromTON(defaultTONExecutorAmount), | ||
| } | ||
|
|
||
| return ton.NewTimelockExecutor(opts) |
Copilot
AI
Jan 27, 2026
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.
The TON timelock executor creation in getTimelockExecutorWithChainOverride lacks test coverage. Add test cases to verify the timelock executor is created correctly with the expected options, similar to other chain family test cases.
| chain := cfg.blockchains.TonChains()[cfg.chainSelector] | ||
| inspector = ton.NewInspector(chain.Client) |
Copilot
AI
Jan 27, 2026
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.
Variable 'chain' should be renamed to 'c' for consistency with the naming pattern used in the same function for other chain families (lines 1535, 1539, 1542, 1545).
| chain := cfg.blockchains.TonChains()[cfg.chainSelector] | |
| inspector = ton.NewInspector(chain.Client) | |
| c := cfg.blockchains.TonChains()[cfg.chainSelector] | |
| inspector = ton.NewInspector(c.Client) |
| } | ||
| case chainsel.FamilyTon: | ||
| chain := cfg.blockchains.TonChains()[cfg.chainSelector] | ||
| inspector = ton.NewInspector(chain.Client) |
Copilot
AI
Jan 27, 2026
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.
The TON inspector creation in getInspectorFromChainSelector lacks test coverage. Add test cases to verify the inspector is created correctly for TON chains.
gustavogama-cll
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.
re-approving mcms changes
chain/ton/provider/rpc_provider.go
Outdated
| } | ||
|
|
||
| // buildChain creates a ton.Chain with the given parameters. | ||
| func buildChain(selector uint64, api *tonlib.APIClient, tonWallet *wallet.Wallet, httpURL string) *ton.Chain { |
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.
Not sure what's the purpose of this method, which just instantiates the type vs just doing it without the method? Then we also have a Test_buildChain test for this method? Seems we could do without.
b25e6bc
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
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…rt-ton-timelock-converter
…rt-ton-timelock-converter
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
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| defaultProposalValidity = 72 * time.Hour | ||
|
|
||
| // defaultTONExecutorAmount is the default amount of TON for MCMS/Timelock executor transactions. | ||
| // This is a static estimate that should be sufficient for most operations. |
Copilot
AI
Jan 29, 2026
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.
The comment describes this as a 'static estimate' but doesn't explain why 0.1 TON was chosen or what factors influence whether this amount is sufficient. Consider adding context about what operations this covers and approximate cost considerations.
| // This is a static estimate that should be sufficient for most operations. | |
| // This is a conservative static estimate intended to cover the gas and storage fees for a | |
| // typical MCMS/Timelock executor flow on TON (e.g. submitting and executing a proposal with | |
| // a small to medium number of actions) under current mainnet fee conditions, with an extra | |
| // safety buffer. The 0.1 TON value was chosen based on observed costs for common operations | |
| // plus headroom to account for moderate message size growth and fee fluctuations. | |
| // | |
| // IMPORTANT: This may be insufficient for unusually large/complex proposals, for networks | |
| // or environments with significantly higher fees, or if TON fee dynamics change over time. | |
| // Operators who routinely submit high-complexity proposals should consider overriding this | |
| // default with a higher value until a dynamic gas estimation mechanism is available. | |
| // |
| case chainsel.FamilyTon: | ||
| encoder, ok := encoder.(*ton.Encoder) | ||
| if !ok { | ||
| return nil, fmt.Errorf("invalid encoder type for TON chain %d: expected *ton.Encoder, got %T", chainSelector, encoder) |
Copilot
AI
Jan 29, 2026
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.
The error message for TON encoder type validation is more detailed than the equivalent error messages for EVM (line 1451), Solana (line 1460), Aptos (line 1468), and Sui (line 1481) chains, which only say 'invalid encoder type: %T'. Consider making error messages consistent across all chain families.
| return nil, fmt.Errorf("invalid encoder type for TON chain %d: expected *ton.Encoder, got %T", chainSelector, encoder) | |
| return nil, fmt.Errorf("invalid encoder type: %T", encoder) |
| p := &RPCChainProvider{ | ||
| selector: 123, | ||
| config: RPCChainProviderConfig{ | ||
| HTTPURL: "", // invalid - missing URL |
Copilot
AI
Jan 29, 2026
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.
The comment says 'invalid - missing URL' but an empty string is technically a valid Go string value. Consider clarifying that this is testing validation logic for an empty/missing URL configuration.
| HTTPURL: "", // invalid - missing URL | |
| HTTPURL: "", // invalid: empty URL to test validation of missing/empty configuration |
|




Jira.
Adding TON timelock converter support. Note the TON support for executor override is also added here, but there's another PR for supporting the migration to MCMS-Tools repo: https://github.com/smartcontractkit/mcms-tools/pull/261
Need to add test coverage