fix: update to filter by VM for evm chains requiring TSS funds migration #4502
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe PR renames the TSS migration function from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #4502 +/- ##
===========================================
- Coverage 63.92% 63.88% -0.05%
===========================================
Files 472 472
Lines 29354 29354
===========================================
- Hits 18766 18752 -14
- Misses 9554 9561 +7
- Partials 1034 1041 +7
🚀 New features to boost your workflow:
|
kingpinXD
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
x/crosschain/keeper/msg_server_update_tss.go (2)
42-44: Minor formatting issue in comment.There's a missing space in the comment: "Sui,Ton" should be "Sui, Ton" for consistency.
🔎 Suggested fix:
- // Solana, Sui,Ton do not need funds migration just updating TSS address is enough + // Solana, Sui, Ton do not need funds migration; updating TSS address is sufficient
77-82: Comment is inconsistent with implementation.The comment on line 81 states "Consensus is bitcoin or ethereum" but the implementation now uses
FilterByVM(chains.Vm_evm)for EVM chains rather thanFilterByConsensus(chains.Consensus_ethereum). This discrepancy could lead to confusion for future maintainers.🔎 Suggested fix:
// TSSFundsMigrationChains returns the chains that support tss migration. // Chains that support tss migration are chains that have the following properties: // 1. External chains // 2. Gateway observer -// 3. Consensus is bitcoin or ethereum (Other consensus types are not supported) +// 3. VM is EVM or Consensus is bitcoin (Solana, Sui, TON are excluded as they do not require funds migration)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
changelog.md(1 hunks)x/crosschain/keeper/msg_server_update_tss.go(3 hunks)x/crosschain/keeper/msg_server_update_tss_test.go(13 hunks)x/crosschain/simulation/operation_update_tss_address.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
Files:
x/crosschain/keeper/msg_server_update_tss.gox/crosschain/keeper/msg_server_update_tss_test.gox/crosschain/simulation/operation_update_tss_address.go
🧠 Learnings (11)
📓 Common learnings
Learnt from: lumtis
Repo: zeta-chain/node PR: 4199
File: zetaclient/chains/evm/signer/signer_admin.go:25-26
Timestamp: 2025-09-15T13:42:17.594Z
Learning: In PR 4199, the CLI references to CmdMigrateTssFunds in x/crosschain/client/cli/ files are intentionally not updated as they are out of scope for this specific refactor focused on removing ERC20 custody messages.
Learnt from: kingpinXD
Repo: zeta-chain/node PR: 4064
File: cmd/zetae2e/local/solana.go:0-0
Timestamp: 2025-08-06T01:54:04.100Z
Learning: The `CheckSolanaTSSBalance()` method in e2e/runner has been refactored to not return an error. Instead, it uses internal assertions (require statements) to fail the test immediately if balance checks fail. This is part of a broader refactoring pattern in the E2E runner where balance check methods were changed from error-returning to assertion-based approaches.
📚 Learning: 2025-09-15T13:42:17.594Z
Learnt from: lumtis
Repo: zeta-chain/node PR: 4199
File: zetaclient/chains/evm/signer/signer_admin.go:25-26
Timestamp: 2025-09-15T13:42:17.594Z
Learning: In PR 4199, the CLI references to CmdMigrateTssFunds in x/crosschain/client/cli/ files are intentionally not updated as they are out of scope for this specific refactor focused on removing ERC20 custody messages.
Applied to files:
changelog.mdx/crosschain/keeper/msg_server_update_tss.gox/crosschain/keeper/msg_server_update_tss_test.gox/crosschain/simulation/operation_update_tss_address.go
📚 Learning: 2025-05-30T16:31:30.275Z
Learnt from: skosito
Repo: zeta-chain/node PR: 3939
File: go.mod:52-52
Timestamp: 2025-05-30T16:31:30.275Z
Learning: The ethermint dependency updates in the zeta-chain/node repository are typically moves between feature branches and main branch of the same fork, not breaking API changes. CI status should be verified before assuming compilation issues.
Applied to files:
changelog.md
📚 Learning: 2025-09-08T03:18:14.656Z
Learnt from: ws4charlie
Repo: zeta-chain/node PR: 4165
File: e2e/e2etests/test_spl_deposit_and_call.go:39-41
Timestamp: 2025-09-08T03:18:14.656Z
Learning: For non-EVM chain addresses (like Solana, TON, Sui) in ZetaChain e2e tests, string representations are used for the sender parameter in AssertTestDAppZEVMCalled, while EVM chains use raw byte representations. This is a deliberate architectural choice for consistency across different chain types.
Applied to files:
changelog.md
📚 Learning: 2025-08-06T01:54:04.100Z
Learnt from: kingpinXD
Repo: zeta-chain/node PR: 4064
File: cmd/zetae2e/local/solana.go:0-0
Timestamp: 2025-08-06T01:54:04.100Z
Learning: The `CheckSolanaTSSBalance()` method in e2e/runner has been refactored to not return an error. Instead, it uses internal assertions (require statements) to fail the test immediately if balance checks fail. This is part of a broader refactoring pattern in the E2E runner where balance check methods were changed from error-returning to assertion-based approaches.
Applied to files:
x/crosschain/keeper/msg_server_update_tss.gox/crosschain/keeper/msg_server_update_tss_test.go
📚 Learning: 2024-11-01T10:30:27.952Z
Learnt from: lumtis
Repo: zeta-chain/node PR: 2984
File: x/crosschain/keeper/msg_server_whitelist_erc20.go:39-48
Timestamp: 2024-11-01T10:30:27.952Z
Learning: In the context of whitelisting assets in `MsgWhitelistERC20` within `x/crosschain/keeper/msg_server_whitelist_erc20.go`, there is no specific address validation required for Solana addresses.
Applied to files:
x/crosschain/keeper/msg_server_update_tss.go
📚 Learning: 2024-07-04T23:47:56.072Z
Learnt from: ws4charlie
Repo: zeta-chain/node PR: 2411
File: zetaclient/orchestrator/orchestrator.go:222-237
Timestamp: 2024-07-04T23:47:56.072Z
Learning: The `GetUpdatedChainObserver` method in the `Orchestrator` class is covered by unit tests in `zetaclient/orchestrator/orchestrator_test.go` and `zetaclient/orchestrator/chain_activate_test.go`.
Applied to files:
x/crosschain/keeper/msg_server_update_tss_test.go
📚 Learning: 2024-07-04T23:46:38.428Z
Learnt from: ws4charlie
Repo: zeta-chain/node PR: 2411
File: zetaclient/orchestrator/orchestrator.go:192-217
Timestamp: 2024-07-04T23:46:38.428Z
Learning: The `GetUpdatedSigner` method in `zetaclient/orchestrator/orchestrator.go` is covered by unit tests in `zetaclient/orchestrator/chain_activate_test.go` and `zetaclient/orchestrator/orchestrator_test.go`.
Applied to files:
x/crosschain/keeper/msg_server_update_tss_test.go
📚 Learning: 2024-09-13T22:29:09.747Z
Learnt from: ws4charlie
Repo: zeta-chain/node PR: 2870
File: zetaclient/orchestrator/orchestrator.go:413-413
Timestamp: 2024-09-13T22:29:09.747Z
Learning: When a method is renamed (e.g., `IsUTXO()` to `IsBitcoin()`), and existing tests cover the affected code paths, do not request additional tests for the renamed methods.
Applied to files:
x/crosschain/keeper/msg_server_update_tss_test.go
📚 Learning: 2024-09-23T19:51:43.360Z
Learnt from: ws4charlie
Repo: zeta-chain/node PR: 2907
File: zetaclient/chains/bitcoin/observer/outbound_test.go:81-82
Timestamp: 2024-09-23T19:51:43.360Z
Learning: In the `mineTxNSetNonceMark` function within `bitcoin/observer/outbound_test.go`, it's acceptable to hardcode the chain ID, as the tests do not require varying chain IDs.
Applied to files:
x/crosschain/keeper/msg_server_update_tss_test.go
📚 Learning: 2024-12-12T15:51:51.144Z
Learnt from: kingpinXD
Repo: zeta-chain/node PR: 3207
File: x/observer/simulation/operation_reset_chain_nonces.go:26-27
Timestamp: 2024-12-12T15:51:51.144Z
Learning: In `x/observer/simulation/operation_reset_chain_nonces.go`, within the `SimulateResetChainNonces` function, the `authAccount` cannot be nil because it corresponds to the policy account which is set using simulation accounts.
Applied to files:
x/crosschain/simulation/operation_update_tss_address.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: test-sim-fullappsimulation / sim
- GitHub Check: test-sim-after-import / sim
- GitHub Check: test-sim-import-export / sim
- GitHub Check: test-sim-nondeterminism / sim
- GitHub Check: Cursor Bugbot
- GitHub Check: build-zetanode
- GitHub Check: lint
- GitHub Check: gosec
- GitHub Check: build-and-test
- GitHub Check: build
- GitHub Check: semgrep/ci
- GitHub Check: analyze (go)
🔇 Additional comments (7)
changelog.md (1)
26-26: LGTM!The changelog entry follows the established format and accurately describes the fix for TSS funds migration filtering.
x/crosschain/keeper/msg_server_update_tss.go (1)
84-95: LGTM!The filtering logic correctly identifies chains requiring TSS funds migration by combining EVM chains (via VM type) and Bitcoin chains (via consensus type). This approach properly includes all EVM-based chains while excluding Solana, Sui, and TON which do not require funds migration.
x/crosschain/simulation/operation_update_tss_address.go (1)
31-38: LGTM!The simulation correctly uses the renamed
TSSFundsMigrationChains(ctx)function, maintaining consistency with the keeper implementation.x/crosschain/keeper/msg_server_update_tss_test.go (4)
69-83: LGTM!Test correctly updated to use
TSSFundsMigrationChains(ctx). The assertion verifying migrator count matches the number of chains requiring migration is sound.
334-364: LGTM!The test comprehensively validates that only EVM and Bitcoin chains are returned, with proper exclusion of Solana, Sui, TON (catchain), and Tendermint consensus chains.
366-437: LGTM!Excellent test coverage for mainnet chains. The test explicitly validates:
- Expected chains (Ethereum, BSC, Bitcoin, Polygon, Base, Arbitrum, Avalanche) are included
- Excluded chains (ZetaChain, Solana, Sui, TON) are properly filtered out
- Chain properties (external, observers gateway, VM type) are verified
439-515: LGTM!The testnet test case mirrors the mainnet structure and provides thorough validation for testnet-specific chains including multiple Bitcoin testnets (Signet, Testnet4) and various EVM testnets. Excluded chains (ZetaChainTestnet, SolanaDevnet, SuiTestnet, TONTestnet) are properly verified.
Description
This PR updates the check for external chains that need TSS funds migration to use VM_EVM.
This would now include all EVM-based chains
How Has This Been Tested?
Note
Switches TSS funds migration eligibility to VM-based EVM filtering and updates keeper, tests, simulation, and changelog accordingly.
GetChainsSupportingTSSMigrationtoTSSFundsMigrationChainsand change EVM filtering tochains.FilterByVM(chains.Vm_evm)(keeps Bitcoin via consensus filter).TSSFundsMigrationChains; minor comment tweak (btc and evm).TSSFundsMigrationChains.TSSFundsMigrationChainsinx/crosschain/simulation/operation_update_tss_address.go.Written by Cursor Bugbot for commit 19298d4. Configure here.
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.