fix(trp): adjust interface to match available mempool data#910
Merged
fix(trp): adjust interface to match available mempool data#910
Conversation
📝 WalkthroughWalkthroughThis PR adds comprehensive integration tests for mempool confirm/drop policies, removes redundant unit tests, and extends transaction tracking structures with confirmation and stage information to support improved state reporting. Changes
Sequence DiagramsequenceDiagram
participant Test as Integration Test
participant Mempool as EphemeralMempool/<br/>RedbMempool
participant TxState as Transaction State
Test->>Mempool: Create transaction
Mempool->>TxState: Store in pending state
Test->>Mempool: Call acknowledge()
Mempool->>TxState: Transition to acknowledged
Test->>Mempool: Advance block N times
Mempool->>TxState: Increment non_confirmations
alt Threshold Met (≥6 blocks)
Mempool->>TxState: Mark as finalized
else Below Threshold
Mempool->>TxState: Remain in pending state
end
Test->>Mempool: Verify finalized log / status
Mempool-->>Test: Return transaction state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/mempool.rs (2)
413-495: Consider a macro to reduce test boilerplate.The 16 test functions follow an identical pattern (create store, call assertion). A simple macro could cut the repetition:
macro_rules! mempool_tests { ($name:ident, $store_expr:expr) => { paste::paste! { #[test] fn [<$name _finalize_after_threshold>]() { assert_finalize_after_threshold(&$store_expr); } #[test] fn [<$name _drop_after_threshold>]() { assert_drop_after_threshold(&$store_expr); } // ... etc. } }; } mempool_tests!(ephemeral, EphemeralMempool::new()); mempool_tests!(redb, RedbMempool::in_memory().unwrap());This is entirely optional — the current explicit form is also perfectly readable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mempool.rs` around lines 413 - 495, The tests repeat identical patterns for EphemeralMempool and RedbMempool; introduce a macro (e.g., macro_rules! mempool_tests) that takes a base name and a store expression and generates the set of #[test] functions calling assert_finalize_after_threshold, assert_drop_after_threshold, assert_confirm_resets_non_confirmations, assert_rollback_to_pending, assert_re_confirm_after_rollback, assert_not_finalized_before_threshold, assert_not_dropped_before_threshold, and assert_mixed_confirm for that store; use paste::paste (or another identifier-concatenation helper) inside the macro to create unique test function names, then replace the 16 explicit tests with two macro invocations like mempool_tests!(ephemeral, EphemeralMempool::new()) and mempool_tests!(redb, RedbMempool::in_memory().unwrap()).
12-14: Production threshold constants are private and cannot be imported directly.The constants
MEMPOOL_FINALIZE_THRESHOLDandMEMPOOL_DROP_THRESHOLDincrates/core/src/sync.rsare not publicly exported, so the suggested import is not feasible without first making them public in the core crate.The duplication concern remains valid: if production thresholds change, these tests will silently use stale values. Either:
- Make the constants public in
dolos_coreand import them here, OR- Keep them duplicated but add a runtime assertion that compares against production values where possible, OR
- Document the coupling explicitly to prevent undetected drift.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mempool.rs` around lines 12 - 14, The test duplicates production thresholds that are private in crates/core/src/sync.rs; make the production constants MEMPOOL_FINALIZE_THRESHOLD and MEMPOOL_DROP_THRESHOLD pub (e.g., change their declarations to pub const in sync.rs) and then import and use those symbols in tests/mempool.rs instead of hardcoding FINALIZE_THRESHOLD / DROP_THRESHOLD; alternatively, if you prefer not to change visibility, add a runtime assertion in tests/mempool.rs that explicitly documents the coupling and compares the test constants against the authoritative values (exposed via a new accessor function or debug-only pub fn) so test drift is detected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/trp/src/methods.rs`:
- Line 126: The serialized string for the enum variant
dolos_core::MempoolTxStage::RolledBack was changed to "rolled_back", causing a
breaking API change; revert the mapping in the serialization logic back to the
previous value (e.g., "pending") or, if the intent is to introduce the new
value, update all downstream consumers and documentation accordingly — locate
the match/serialization code that maps MempoolTxStage::RolledBack and either
restore the original string literal used previously or coordinate updating
clients and docs to accept "rolled_back".
---
Nitpick comments:
In `@tests/mempool.rs`:
- Around line 413-495: The tests repeat identical patterns for EphemeralMempool
and RedbMempool; introduce a macro (e.g., macro_rules! mempool_tests) that takes
a base name and a store expression and generates the set of #[test] functions
calling assert_finalize_after_threshold, assert_drop_after_threshold,
assert_confirm_resets_non_confirmations, assert_rollback_to_pending,
assert_re_confirm_after_rollback, assert_not_finalized_before_threshold,
assert_not_dropped_before_threshold, and assert_mixed_confirm for that store;
use paste::paste (or another identifier-concatenation helper) inside the macro
to create unique test function names, then replace the 16 explicit tests with
two macro invocations like mempool_tests!(ephemeral, EphemeralMempool::new())
and mempool_tests!(redb, RedbMempool::in_memory().unwrap()).
- Around line 12-14: The test duplicates production thresholds that are private
in crates/core/src/sync.rs; make the production constants
MEMPOOL_FINALIZE_THRESHOLD and MEMPOOL_DROP_THRESHOLD pub (e.g., change their
declarations to pub const in sync.rs) and then import and use those symbols in
tests/mempool.rs instead of hardcoding FINALIZE_THRESHOLD / DROP_THRESHOLD;
alternatively, if you prefer not to change visibility, add a runtime assertion
in tests/mempool.rs that explicitly documents the coupling and compares the test
constants against the authoritative values (exposed via a new accessor function
or debug-only pub fn) so test drift is detected.
| dolos_core::MempoolTxStage::Finalized => "finalized", | ||
| dolos_core::MempoolTxStage::Dropped => "dropped", | ||
| dolos_core::MempoolTxStage::RolledBack => "pending", | ||
| dolos_core::MempoolTxStage::RolledBack => "rolled_back", |
There was a problem hiding this comment.
Breaking API change: RolledBack stage string changed.
This changes the serialized representation from (presumably) "pending" to "rolled_back". Any API consumer checking the stage field for rolled-back transactions will see a different value. Make sure downstream clients and documentation are updated accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/trp/src/methods.rs` at line 126, The serialized string for the enum
variant dolos_core::MempoolTxStage::RolledBack was changed to "rolled_back",
causing a breaking API change; revert the mapping in the serialization logic
back to the previous value (e.g., "pending") or, if the intent is to introduce
the new value, update all downstream consumers and documentation accordingly —
locate the match/serialization code that maps MempoolTxStage::RolledBack and
either restore the original string literal used previously or coordinate
updating clients and docs to accept "rolled_back".
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Tests