refactor: cleanup mempool fork #4060
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 📝 WalkthroughWalkthroughThis change removes the custom priority nonce mempool fork and its related proposal handler, tests, and sender extraction utilities, replacing them with the standard Cosmos SDK priority mempool and a custom Ethereum signer extraction adapter. The application initialization is updated accordingly, and dependencies are bumped to support the new approach. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant SDKMempool
participant EthSignerAdapter
participant DefaultSignerAdapter
App->>SDKMempool: Initialize PriorityMempool (with EthSignerAdapter)
App->>EthSignerAdapter: GetSigners(tx)
alt Ethereum Tx
EthSignerAdapter->>App: Extract Ethereum sender/nonce
EthSignerAdapter-->>SDKMempool: Return Ethereum signer(s)
else Non-Ethereum Tx
EthSignerAdapter->>DefaultSignerAdapter: GetSigners(tx)
DefaultSignerAdapter-->>EthSignerAdapter: Return default signer(s)
EthSignerAdapter-->>SDKMempool: Return default signer(s)
end
App->>SDKMempool: Set as mempool in BaseApp
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope functional code changes were identified. All modifications directly support the migration away from the custom mempool and adoption of the standard Cosmos SDK mechanisms as specified in the linked issue. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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)
changelog.md (1)
29-29: Fix markdown link formatting.Remove the space between the PR number and URL to properly format the markdown link.
-* [4060] (https://github.com/zeta-chain/node/pull/4060) - cleanup forked mempool code +* [4060](https://github.com/zeta-chain/node/pull/4060) - cleanup forked mempool codeapp/signer.go (1)
12-14: Incorrect comment - this is a custom implementation, not default.The comment incorrectly states this is "the default implementation" when it's actually a custom adapter specifically for Ethereum transaction signer extraction.
-// EthSignerExtractionAdapter is the default implementation of SignerExtractionAdapter. It extracts the signers -// from a cosmos-sdk tx via GetSignaturesV2. +// EthSignerExtractionAdapter is a custom implementation of SignerExtractionAdapter that extracts signers +// from Ethereum transactions within Cosmos SDK transactions, falling back to the default extractor for other transaction types.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
app/app.go(0 hunks)app/signer.go(1 hunks)changelog.md(1 hunks)cmd/zetacored/root.go(3 hunks)go.mod(4 hunks)pkg/mempool/custom_proposal_handler.go(0 hunks)pkg/mempool/custom_proposal_handler_test.go(0 hunks)pkg/mempool/mempool_test.go(0 hunks)pkg/mempool/priority_nonce_mempool.go(0 hunks)pkg/mempool/priority_nonce_mempool_test.go(0 hunks)pkg/mempool/senders_with_nonce.go(0 hunks)
💤 Files with no reviewable changes (7)
- pkg/mempool/senders_with_nonce.go
- app/app.go
- pkg/mempool/custom_proposal_handler_test.go
- pkg/mempool/priority_nonce_mempool.go
- pkg/mempool/mempool_test.go
- pkg/mempool/custom_proposal_handler.go
- pkg/mempool/priority_nonce_mempool_test.go
🧰 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:
app/signer.gocmd/zetacored/root.go
🧠 Learnings (5)
📓 Common learnings
Learnt from: gartnera
PR: zeta-chain/node#3070
File: cmd/zetae2e/init.go:0-0
Timestamp: 2024-10-30T17:56:16.341Z
Learning: In code reviews for Go files like `cmd/zetae2e/init.go` in the ZetaChain project, avoid suggesting unrelated refactoring. Focus comments on changes relevant to the PR objectives.
Learnt from: skosito
PR: zeta-chain/node#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.
Learnt from: gartnera
PR: zeta-chain/node#3228
File: zetaclient/orchestrator/orchestrator.go:388-401
Timestamp: 2024-11-27T22:01:49.732Z
Learning: When reviewing code changes in `zetaclient/orchestrator/orchestrator.go`, avoid suggesting refactoring that is unrelated to the current PR.
Learnt from: gartnera
PR: zeta-chain/node#3228
File: cmd/zetaclientd/start.go:222-231
Timestamp: 2024-11-27T22:02:48.034Z
Learning: In the `zetaclient` codebase, port numbers like `26657` are hardcoded, and changing them to be configurable via the config struct is considered unrelated refactoring.
Learnt from: gartnera
PR: zeta-chain/node#3632
File: zetaclient/chains/solana/signer/signer.go:304-304
Timestamp: 2025-03-04T22:39:58.395Z
Learning: The Solana signer implementation in zetaclient/chains/solana/signer/signer.go has limited test coverage, particularly for the transaction broadcasting logic with fallback scenarios. Adding this coverage has been acknowledged as a potential future improvement outside the scope of immediate fixes.
Learnt from: gartnera
PR: zeta-chain/node#3489
File: pkg/chains/chains.go:0-0
Timestamp: 2025-02-06T15:31:32.723Z
Learning: SuiMainnet uses chain ID 105 (temporarily) instead of 101 to avoid collision with ZetaChainPrivnet. This is tracked in issue #3491 for proper resolution.
changelog.md (10)
Learnt from: gartnera
PR: #3228
File: zetaclient/orchestrator/orchestrator.go:388-401
Timestamp: 2024-11-27T22:01:49.732Z
Learning: When reviewing code changes in zetaclient/orchestrator/orchestrator.go, avoid suggesting refactoring that is unrelated to the current PR.
Learnt from: skosito
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.
Learnt from: gartnera
PR: #3632
File: zetaclient/chains/solana/signer/signer.go:304-304
Timestamp: 2025-03-04T22:39:58.395Z
Learning: The Solana signer implementation in zetaclient/chains/solana/signer/signer.go has limited test coverage, particularly for the transaction broadcasting logic with fallback scenarios. Adding this coverage has been acknowledged as a potential future improvement outside the scope of immediate fixes.
Learnt from: gartnera
PR: #3228
File: cmd/zetaclientd/start.go:222-231
Timestamp: 2024-11-27T22:02:48.034Z
Learning: In the zetaclient codebase, port numbers like 26657 are hardcoded, and changing them to be configurable via the config struct is considered unrelated refactoring.
Learnt from: kingpinXD
PR: #3503
File: e2e/runner/require.go:0-0
Timestamp: 2025-02-07T19:33:35.631Z
Learning: In e2e tests for the zeta-chain/node repository, minimize computation and avoid redundant checks that are already covered by unit tests to ensure faster execution. For example, ballot sorting is verified in unit tests, so e2e tests can safely assume the order.
Learnt from: ws4charlie
PR: #4053
File: e2e/e2etests/test_bitcoin_std_deposit.go:42-42
Timestamp: 2025-07-28T18:08:13.883Z
Learning: In Bitcoin deposit E2E tests for the ZetaChain project, DepositBTCWithAmount is preferred over DepositBTCWithExactAmount because depositing exact amounts to a ZEVM receiver is complex. The tests use rough amounts and then calculate the actual received amount from the raw Bitcoin transaction to verify balance changes, making the tests more robust and less flaky.
Learnt from: ws4charlie
PR: #2411
File: zetaclient/orchestrator/chain_activate.go:184-247
Timestamp: 2024-07-05T00:02:31.446Z
Learning: The CreateSignerObserverBTC function in zetaclient/orchestrator/chain_activate.go is covered by unit tests in zetaclient/orchestrator/chain_activate_test.go.
Learnt from: gartnera
PR: #3070
File: cmd/zetae2e/init.go:0-0
Timestamp: 2024-10-30T17:56:16.341Z
Learning: In code reviews for Go files like cmd/zetae2e/init.go in the ZetaChain project, avoid suggesting unrelated refactoring. Focus comments on changes relevant to the PR objectives.
Learnt from: gartnera
PR: #3489
File: pkg/chains/chains.go:0-0
Timestamp: 2025-02-06T15:31:32.723Z
Learning: SuiMainnet uses chain ID 105 (temporarily) instead of 101 to avoid collision with ZetaChainPrivnet. This is tracked in issue #3491 for proper resolution.
Learnt from: ws4charlie
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.
go.mod (5)
Learnt from: gartnera
PR: #2817
File: contrib/rpcimportable/go.mod:1-3
Timestamp: 2024-09-24T18:43:46.232Z
Learning: In this project, minor version differences in Go modules are acceptable and do not require strict consistency.
Learnt from: gartnera
PR: #2817
File: contrib/rpcimportable/go.mod:5-6
Timestamp: 2024-09-24T18:45:32.028Z
Learning: In the contrib/rpcimportable test, the go.mod file should be empty when committed, and the go.sum file should not be committed, as committing go.sum causes the test to fail.
Learnt from: gartnera
PR: #3395
File: .github/workflows/reusable-sim.yml:29-30
Timestamp: 2025-01-22T22:46:58.820Z
Learning: The zeta-chain/node repository uses Go version >= 1.22. Do not suggest downgrading to earlier versions like 1.21.
Learnt from: gartnera
PR: #2934
File: go.mod:343-343
Timestamp: 2024-09-30T14:26:21.719Z
Learning: Importing different major versions is allowed in Go.
Learnt from: gartnera
PR: #3105
File: cmd/zetae2e/local/bitcoin.go:8-8
Timestamp: 2024-11-06T21:11:34.420Z
Learning: In the e2e codebase, it's acceptable to use github.com/stretchr/testify/require outside of test files.
app/signer.go (5)
Learnt from: gartnera
PR: #3632
File: zetaclient/chains/solana/signer/signer.go:304-304
Timestamp: 2025-03-04T22:39:58.395Z
Learning: The Solana signer implementation in zetaclient/chains/solana/signer/signer.go has limited test coverage, particularly for the transaction broadcasting logic with fallback scenarios. Adding this coverage has been acknowledged as a potential future improvement outside the scope of immediate fixes.
Learnt from: ws4charlie
PR: #2411
File: zetaclient/orchestrator/chain_activate.go:116-181
Timestamp: 2024-07-05T00:02:36.493Z
Learning: The CreateSignerObserverEVM function in zetaclient/orchestrator/chain_activate.go is covered by unit tests in zetaclient/orchestrator/chain_activate_test.go.
Learnt from: ws4charlie
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.
Learnt from: ws4charlie
PR: #2411
File: zetaclient/orchestrator/chain_activate.go:184-247
Timestamp: 2024-07-05T00:02:31.446Z
Learning: The CreateSignerObserverBTC function in zetaclient/orchestrator/chain_activate.go is covered by unit tests in zetaclient/orchestrator/chain_activate_test.go.
Learnt from: gartnera
PR: #3485
File: e2e/utils/sui/signer.go:24-35
Timestamp: 2025-02-06T16:29:58.925Z
Learning: The Sui blockchain requires signatures in [R || S || V] format, which is best generated using go-ethereum's crypto.Sign function that takes an ecdsa.PrivateKey.
cmd/zetacored/root.go (9)
Learnt from: gartnera
PR: #3228
File: cmd/zetaclientd/start.go:222-231
Timestamp: 2024-11-27T22:02:48.034Z
Learning: In the zetaclient codebase, port numbers like 26657 are hardcoded, and changing them to be configurable via the config struct is considered unrelated refactoring.
Learnt from: gartnera
PR: #3070
File: cmd/zetae2e/init.go:0-0
Timestamp: 2024-10-30T17:56:16.341Z
Learning: In code reviews for Go files like cmd/zetae2e/init.go in the ZetaChain project, avoid suggesting unrelated refactoring. Focus comments on changes relevant to the PR objectives.
Learnt from: ws4charlie
PR: #2987
File: pkg/memo/codec.go:55-64
Timestamp: 2024-10-10T18:18:17.785Z
Learning: In the pkg/memo package, the functions NewCodecABI and NewCodecCompact are defined and accessible within the package scope, eliminating the need for explicit import statements in this file.
Learnt from: ws4charlie
PR: #2987
File: pkg/memo/codec_abi.go:0-0
Timestamp: 2024-10-10T18:50:23.406Z
Learning: The constant ABIAlignment has been moved from pkg/memo/codec_abi.go to the testutil package.
Learnt from: gartnera
PR: #2817
File: pkg/sdkconfig/sdkconfig.go:29-36
Timestamp: 2024-09-24T18:45:45.448Z
Learning: In pkg/sdkconfig/sdkconfig.go, the current implementation of the Set and SetDefault functions is intentional, and further refactoring is unnecessary.
Learnt from: gartnera
PR: #3228
File: zetaclient/orchestrator/orchestrator.go:388-401
Timestamp: 2024-11-27T22:01:49.732Z
Learning: When reviewing code changes in zetaclient/orchestrator/orchestrator.go, avoid suggesting refactoring that is unrelated to the current PR.
Learnt from: gartnera
PR: #3077
File: cmd/zetae2e/local/local.go:188-191
Timestamp: 2024-10-31T21:15:55.881Z
Learning: In Go file cmd/zetae2e/local/local.go, the network is fully functional before calling FundEmissionsPool(), so retry logic is unnecessary.
Learnt from: ws4charlie
PR: #2987
File: testutil/sample/memo.go:0-0
Timestamp: 2024-10-10T18:54:33.554Z
Learning: In the testutil/sample/memo.go file, unchecked type assertions in the ABIPack function are acceptable because it is designed for unit tests and simplicity.
Learnt from: gartnera
PR: #3395
File: .github/workflows/reusable-sim.yml:29-30
Timestamp: 2025-01-22T22:46:58.820Z
Learning: The zeta-chain/node repository uses Go version >= 1.22. Do not suggest downgrading to earlier versions like 1.21.
🪛 markdownlint-cli2 (0.17.2)
changelog.md
29-29: Bare URL used
(MD034, no-bare-urls)
⏰ 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). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: start-upgrade-test / e2e
🔇 Additional comments (3)
go.mod (1)
20-20: Dependency updates align with PR objectives.The Cosmos SDK upgrade to v0.53.4 successfully incorporates the security patches mentioned in the PR description. The accompanying dependency updates for
spf13/cast,spf13/pflag, andsigs.k8s.io/yamlare appropriate transitive updates that maintain compatibility.Also applies to: 49-52, 298-298, 345-345
app/signer.go (1)
10-43: Well-structured Ethereum signer extraction implementation.The adapter correctly identifies Ethereum transactions by checking for the specific extension type and extracts the signer data appropriately. The fallback mechanism ensures compatibility with standard Cosmos SDK transactions.
cmd/zetacored/root.go (1)
281-308: Successful migration to standard SDK mempool implementation.The refactoring correctly replaces the custom mempool with the standard Cosmos SDK priority mempool while maintaining Ethereum transaction support through the custom signer extraction adapter. The explicit proposal handler setup ensures proper transaction processing in the consensus layer.
Description
with cosmos 0.53.4 this issue with proposal handler was fixed cosmos/cosmos-sdk#25008 so we dont need to maintain forked code anymore
this also bumps to 0.53.4 including changes from 0.53.3 that we need because of security patch https://github.com/cosmos/cosmos-sdk/releases/tag/v0.53.3
closes #2156
How Has This Been Tested?
Summary by CodeRabbit
Refactor
Dependency Updates
Chores