Skip to content

Conversation

@huangzhen1997
Copy link
Contributor

@huangzhen1997 huangzhen1997 commented Dec 10, 2025

Copilot AI review requested due to automatic review settings December 10, 2025 03:08
@huangzhen1997 huangzhen1997 requested a review from a team as a code owner December 10, 2025 03:08
@changeset-bot
Copy link

changeset-bot bot commented Dec 10, 2025

🦋 Changeset detected

Latest commit: bd55ade

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
chainlink-deployments-framework Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@huangzhen1997 huangzhen1997 marked this pull request as draft December 10, 2025 03:08
Copy link
Contributor

Copilot AI left a 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 adds TON blockchain analyzer support to the chainlink-deployments-framework. The implementation follows existing patterns for other chain families (Aptos, Sui, Solana) by integrating TON-specific MCMS SDK functionality.

Key changes:

  • Added TON transaction analysis capabilities with decoder integration
  • Updated dependency versions including Go runtime, MCMS SDK, and various libraries
  • Refactored transaction encoding logic to use family-based routing

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.

File Description
go.mod Updated Go version to 1.25.3 and bumped various dependencies including MCMS SDK, TON utilities, and added chainlink-ton package
experimental/analyzer/upf/upf.go Integrated TON family support in UPF decoding with mcmstonsdk decoder and simplified encoding logic
experimental/analyzer/ton_analyzer.go New file implementing TON transaction analysis functions mirroring Sui/Aptos patterns
experimental/analyzer/report_builder.go Added TON family handling in proposal and timelock report builders

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@smartcontractkit smartcontractkit deleted a comment from github-actions bot Dec 10, 2025
@huangzhen1997 huangzhen1997 marked this pull request as ready for review December 12, 2025 02:01
Copy link
Contributor

Copilot AI left a 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 8 out of 9 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI review requested due to automatic review settings December 12, 2025 04:00
Copy link
Contributor

Copilot AI left a 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 9 out of 10 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

Copilot AI left a 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 9 out of 10 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


// Sui: mcms::timelock_schedule_batch, mcms::timelock_bypasser_execute_batch
// Aptos: package::module::timelock_schedule_batch, package::module::timelock_bypasser_execute_batch
// TON: ContractType::ScheduleBatch(0x...), ContractType::BypasserExecuteBatch(0x...)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WIll TON type will be provided like this? (e.g., "com.chainlink.ton.lib.access.RBAC::GrantRole(0x0)")

Copy link
Contributor Author

@huangzhen1997 huangzhen1997 Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't check the contract type, because with the timelock converter it would be converted to RBACTimelock::ScheduleBatch(0x0)

Comment on lines +210 to +211
expectedMsg: "EVM registry is not available",
wantErr: true,
Copy link

@krebernisak krebernisak Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Can't this be simplified as wantErr: string which if set is the expected error string?

name: "TON_decode_failure",
selector: chainsel.TON_TESTNET.Selector,
expectedMsg: "failed to decode TON transaction",
wantErr: false, // TON doesn't unmarshal AdditionalFields, so decode errors go to Method field

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, not sure why is here a difference for TON? There shouldn't be one...

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't SUI impl also surfaces errors via .Method member:

Copy link
Contributor Author

@huangzhen1997 huangzhen1997 Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is for default unhappy pass, where the analyze function suppose to fail immediately. Unlike SUI we don't need extra field Unmarshal, so the first error will be hiding in Method field.

name: "TON_decode_failure",
selector: chainsel.TON_TESTNET.Selector,
expectedMsg: "failed to decode TON transaction",
wantErr: false, // TON doesn't unmarshal AdditionalFields, so decode errors go to Method field

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't diverge from other test cases

Copy link
Contributor

@gustavogama-cll gustavogama-cll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't forget to add a changeset and a proper comment to the PR's description.

Copilot AI review requested due to automatic review settings December 21, 2025 16:37
Copy link
Contributor

Copilot AI left a 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 9 out of 10 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

}
}

func TestUpfConvertTimelockProposalWithTon(t *testing.T) {
Copy link

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function name 'TestUpfConvertTimelockProposalWithTon' is inconsistent with Go naming conventions. 'Ton' should be 'TON' to match the established pattern in other test names like 'TestUpfConvertTimelockProposalWithSui'.

Suggested change
func TestUpfConvertTimelockProposalWithTon(t *testing.T) {
func TestUpfConvertTimelockProposalWithTON(t *testing.T) {

Copilot uses AI. Check for mistakes.
name: "TON_decode_failure",
selector: chainsel.TON_TESTNET.Selector,
expectedMsg: "failed to decode TON transaction",
wantErr: false, // TON doesn't unmarshal AdditionalFields, so decode errors go to Method field
Copy link

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment explains TON's behavior of not unmarshaling AdditionalFields, but this is misleading in the context of decode failures. The actual reason decode errors don't return as errors is stated in ton_analyzer.go:48 - it's to prevent blocking the whole proposal. Consider updating the comment to: '// TON returns decode errors in Method field to avoid blocking proposal processing'.

Suggested change
wantErr: false, // TON doesn't unmarshal AdditionalFields, so decode errors go to Method field
wantErr: false, // TON returns decode errors in Method field to avoid blocking proposal processing

Copilot uses AI. Check for mistakes.
name: "TON_decode_failure",
selector: chainsel.TON_TESTNET.Selector,
expectedMsg: "failed to decode TON transaction",
wantErr: false, // TON doesn't unmarshal AdditionalFields, so decode errors go to Method field
Copy link

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as Comment 4 - the comment is misleading about why decode errors go to the Method field. The reason is to avoid blocking proposal processing, not specifically about AdditionalFields unmarshaling.

Suggested change
wantErr: false, // TON doesn't unmarshal AdditionalFields, so decode errors go to Method field
wantErr: false, // decode errors are reported in the Method field so proposal processing is not blocked

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings December 21, 2025 17:23
Copy link
Contributor

Copilot AI left a 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 9 out of 10 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@cl-sonarqube-production
Copy link

ecPablo
ecPablo previously approved these changes Jan 6, 2026
Copy link
Contributor

@ecPablo ecPablo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Left a couple nit comments but nothing blocking

// Instead, put the error message in the Method field so it's visible in the report.
errStr := fmt.Errorf("failed to decode TON transaction: %w", err)

return &DecodedCall{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we should double check this with security. From one side I agree that a failure to decode should not be a blocker for operational activity, but on the other side I know security wants to push for us to reduce blind signing of proposals as much as possible. Will it be common to see decode of operations failing?

// across different chain families (EVM, Solana, Sui, Aptos, TON).
func isTimelockBatchFunction(functionName string) bool {
// EVM function signatures
if functionName == "function scheduleBatch((address,uint256,bytes)[] calls, bytes32 predecessor, bytes32 salt, uint256 delay) returns()" ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we could move this to small per chain family helpers to keep this function a bit more concise and readable

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 - let's mv this to fn per chain (+an interface)


namedArgs, err := toNamedFields(decodedOp)
if err != nil {
return nil, fmt.Errorf("failed to convert decoded operation to named arguments: %w", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we do the same thing of not returning an error and instead putting it on the Decoded call for the same reasons as the comment above?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toNamedFields only fails when there's a keys/arguments mismatch, which indicates a programming bug rather than a transaction decode issue. So I think it would be better to surface this early. Both Sui and Aptos analyzers are returning errors at this point as well.

// across different chain families (EVM, Solana, Sui, Aptos, TON).
func isTimelockBatchFunction(functionName string) bool {
// EVM function signatures
if functionName == "function scheduleBatch((address,uint256,bytes)[] calls, bytes32 predecessor, bytes32 salt, uint256 delay) returns()" ||

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 - let's mv this to fn per chain (+an interface)

"github.com/smartcontractkit/mcms/types"
)

var typeToTLBMap = map[string]lib.TLBMap{

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be available to import now

Copilot AI review requested due to automatic review settings January 22, 2026 02:04
Copy link
Contributor

Copilot AI left a 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 9 out of 9 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 253 to 261
// Use Contains because the opcode suffix (0x...) varies
if strings.Contains(functionName, "::ScheduleBatch(") ||
strings.Contains(functionName, "::BypasserExecuteBatch(") {
return true
}

return false
}

Copy link

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using Contains for TON function matching may result in false positives. Consider using a more precise pattern match or HasPrefix/HasSuffix combination to ensure the function name starts with the contract type and ends with the expected pattern, avoiding matches in the middle of longer function names.

Suggested change
// Use Contains because the opcode suffix (0x...) varies
if strings.Contains(functionName, "::ScheduleBatch(") ||
strings.Contains(functionName, "::BypasserExecuteBatch(") {
return true
}
return false
}
if isTONTimelockBatchFunction(functionName) {
return true
}
return false
}
// isTONTimelockBatchFunction checks if a TON function name corresponds to a timelock batch operation.
// Expected format: ContractType::ScheduleBatch(0x...), ContractType::BypasserExecuteBatch(0x...)
// We split on the first "::" and then require the function part to start with the expected pattern,
// avoiding matches in the middle of longer function names.
func isTONTimelockBatchFunction(functionName string) bool {
parts := strings.SplitN(functionName, "::", 2)
if len(parts) != 2 {
return false
}
contractType := parts[0]
functionPart := parts[1]
if contractType == "" {
return false
}
return strings.HasPrefix(functionPart, "ScheduleBatch(") ||
strings.HasPrefix(functionPart, "BypasserExecuteBatch(")
}

Copilot uses AI. Check for mistakes.
huangzhen1997 and others added 2 commits January 21, 2026 20:35
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings January 22, 2026 02:36
Copy link
Contributor

Copilot AI left a 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 9 out of 10 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI review requested due to automatic review settings January 22, 2026 03:01
Copy link
Contributor

Copilot AI left a 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 10 out of 11 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@cl-sonarqube-production
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants