Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefactors dev-fee internals and tests in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
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 |
|
Can you add the tag |
simply git tag the commit with that particular tag and push it! git tag run-mutation |
sergey3bv
force-pushed
the
test/mut-dev-fee
branch
2 times, most recently
from
b26bb41 to
8b4d8c9
Compare
|
Sigh! Seems not triggering the action, i will check asap. |
|
I run locally the mutations test with: Looks good apart this, try to fix it in case: ---- util::tests::test_get_nostr_client_failure stdout ---- thread 'util::tests::test_get_nostr_client_failure' (25788) panicked at src\util.rs:1458:9: |
probably is not triggered from fork, but since locally just one mutants fails I think we can merge and fix last one. What do you think @grunch ? |
|
@sergey3bv seems your comment disappeared... |
Yeah, I've figured that out myself and did not want to pollute comment section. Since there is time I will try to fix the failing case |
@sergey3bv please don't delete comments, it's hard to follow what is happening if you only see one part of the history |
|
@sergey3bv did you try running locally:
seems worse than yesterday, please check on your side, much more mutants with errors: INFO Auto-set test timeout to 145s
MISSED src/app/dev_fee.rs:147:21: replace += with *= in cleanup_stale_pending_markers in 12s build + 29s test
MISSED src/app/dev_fee.rs:176:20: replace > with == in cleanup_stale_pending_markers in 12s build + 28s test
MISSED src/app/dev_fee.rs:176:20: replace > with < in cleanup_stale_pending_markers in 11s build + 28s test
MISSED src/app/dev_fee.rs:176:20: replace > with >= in cleanup_stale_pending_markers in 11s build + 28s test
MISSED src/app/dev_fee.rs:194:5: replace verify_confirmed_orders with () in 17s build + 28s test
MISSED src/app/dev_fee.rs:250:5: replace recover_partial_payments with () in 16s build + 31s test
MISSED src/app/dev_fee.rs:342:5: replace process_new_dev_fee_payments with () in 18s build + 30s test
MISSED src/app/dev_fee.rs:522:5: replace handle_payment_failure with () in 17s build + 29s test
MISSED src/app/dev_fee.rs:545:5: replace handle_payment_timeout with () in 16s build + 30s test
MISSED src/app/dev_fee.rs:639:20: replace match guard !h.starts_with("PENDING-") with true in check_dev_fee_payment_status in 14s build + 30s test
MISSED src/app/dev_fee.rs:639:20: replace match guard !h.starts_with("PENDING-") with false in check_dev_fee_payment_status in 15s build + 28s test
MISSED src/app/dev_fee.rs:639:20: delete ! in check_dev_fee_payment_status in 15s build + 29s test
MISSED src/app/dev_fee.rs:668:13: delete match arm PaymentStatus::Succeeded in check_dev_fee_payment_status in 11s build + 28s test
MISSED src/app/dev_fee.rs:669:13: delete match arm PaymentStatus::InFlight in check_dev_fee_payment_status in 11s build + 28s test
MISSED src/app/dev_fee.rs:670:13: delete match arm PaymentStatus::Failed in check_dev_fee_payment_status in 12s build + 32s test
test src/app/dev_fee.rs:754:5: replace resolve_dev_fee_invoice -> Result<(String, String), MostroError> with Ok((String::new(), "xyzzy".into())) ... 0s I know that some mutants could be more difficult for mock up setup in case there is an attribute to skip that functions |
sergey3bv
force-pushed
the
test/mut-dev-fee
branch
from
0865b8b to
4956440
Compare
|
Good job! At least functions without $ cargo mutants -f src/app/dev_fee.rs
Found 28 mutants to test
ok Unmutated baseline in 128s build + 28s test
INFO Auto-set test timeout to 144s
28 mutants tested in 22m: 28 caught |
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
Strict Code Review: PR #675 — Mutation testing for dev fee module
This PR bundles three conceptually independent changes: (A) mutation-testing annotations + new unit tests for dev_fee.rs, (B) removal of Mostro pubkey from the source tag in nip33.rs/util.rs, and (C) documentation updates for removed DB encryption. I'll address each.
🔴 HIGH — Three unrelated changes in one PR
This PR mixes:
- Mutation testing for dev_fee (the stated purpose)
- Source tag pubkey removal (
nip33.rs,util.rs,release.rs) - DB encryption removal docs (
docker/,docs/,proto/,examples/)
These should be separate PRs. The source tag change is a protocol-level breaking change (clients parsing &mostro= from the source tag will stop finding it). Bundling it with test improvements makes it harder to review, revert, or bisect.
Recommendation: Split into at least 2 PRs: one for mutation testing, one for the source tag + encryption docs.
🔴 HIGH — Source tag pubkey removal is a breaking change without protocol update
The create_source_tag function now emits:
mostro:{order_id}?relays={relay1,relay2,...}
instead of:
mostro:{order_id}?relays={relay1,relay2,...}&mostro={pubkey}
The deleted docs/SOURCE_TAG_PUBKEY.md explicitly referenced MostroP2P/mobile#541 for client implementation. If any client depends on the &mostro= parameter for instance identification, this is a silent breaking change.
Questions:
- Was
mobile#541implemented? If so, does the mobile client handle absence of&mostro=? - Should the protocol spec be updated to reflect the new format?
🟡 MEDIUM — Unused dependencies added to Cargo.toml
rpassword = "7.3.1"
argon2 = "0.5"
secrecy = "0.10.0"grep -rn "rpassword\|argon2\|secrecy" src/ --include="*.rs" returns zero results. These dependencies are not used anywhere in the codebase. Adding unused deps increases compile time, attack surface, and audit burden.
Recommendation: Remove from this PR. Add them in the PR that actually uses them.
🟡 MEDIUM — ~200 lines of tests removed from nip33.rs without replacement
The PR deletes comprehensive tests:
order_to_tags_emits_y_tag_with_mostro_as_first_valueorder_to_tags_y_tag_matches_platform_helper_outputorder_to_tags_source_tag_includes_mostro_pubkeyinfo_to_tags_emits_y_tag_with_mostro_as_first_valueinfo_to_tags_y_tag_matches_platform_helper_outputdispute_event_tags_emit_y_tag_matching_platform_helperdev_fee_audit_event_tags_emit_y_tag_matching_platform_helper
The mostro_pubkey parameter was removed from order_to_tags, but the y-tag and source-tag logic still exists. The tests for y-tag emission (info_to_tags, dispute events, dev-fee audit events) are not related to the pubkey removal and should not be deleted.
Recommendation: Restore tests that don't depend on mostro_pubkey parameter (at minimum: info_to_tags_*, dispute, dev-fee audit y-tag tests).
🟡 MEDIUM — Tests removed from util.rs without explanation
Three tests removed:
test_get_nostr_client_failuretest_get_nostr_client_successtest_send_dm
These test production code (get_nostr_client, send_dm) that still exists. The commit message doesn't explain why they were removed.
Recommendation: Either rewrite to not depend on global state, or document why the coverage loss is acceptable.
🟡 MEDIUM — MOSTRO_DB_PASSWORD added to compose.yml but code says encryption is removed
docker/compose.yml adds:
environment:
MOSTRO_DB_PASSWORD: ${MOSTRO_DB_PASSWORD-}But docs/STARTUP_AND_CONFIG.md says:
// Security (MOSTRO_DB_PASSWORD unused; database encryption was removed)
pub static MOSTRO_DB_PASSWORD: OnceLock<String> = OnceLock::new();And docker/README.md says the variable is "no longer used." This is contradictory — why add an env var to compose.yml that's documented as unused?
Recommendation: Either remove from compose.yml or explain the migration path.
🟢 LOW — cleanup_stale_pending_markers logs at info! for zero resets
info!(
"Stale PENDING cleanup completed: reset {} orders (TTL: {}s)",
stale_count, CLEANUP_TTL_SECS
);This runs on every cycle (every DEV_FEE_CHECK_INTERVAL_SECS). If the interval is short, this generates log noise when stale_count == 0. The previous version only logged when stale_count > 0, which is better.
Recommendation: Use debug! for zero resets, warn! or info! only when stale_count > 0.
🟢 LOW — RPC_RATE_LIMITING.md rewrites obscure backward-compat nuance
The old docs explicitly stated that validate_db_password never calls record_failure, so the rate limiter's exponential backoff is never triggered by this endpoint. The new docs present the rate limiter as if it actively protects this endpoint, which is misleading.
✅ Positive aspects
- New tests are excellent — Atomic claim, stale cleanup, payment success/failure, edge cases for
parse_pending_timestampare thorough #[mutants::skip]annotations are well-placed on LN-dependent codecleanup_stale_pending_markersreturningu32makes it testable — good refactorrelease_pending_claim_clears_matching_marker_onlytest catches an important safety property
Summary — Requested Changes
Must fix:
- Remove unused dependencies (
rpassword,argon2,secrecy) - Restore deleted tests not related to pubkey removal (y-tag, info_to_tags, dispute, dev-fee audit)
- Resolve compose.yml vs docs contradiction for
MOSTRO_DB_PASSWORD
Strongly recommended:
- Split PR: mutation tests vs source tag removal vs encryption docs
- Confirm no clients depend on
&mostro=in source tag before removing - Restore or replace
test_get_nostr_client_*andtest_send_dm
Nice to have:
- Conditional log level for stale cleanup (debug when 0)
- Clarify rate limiter docs accuracy
|
@sergey3bv maybe there ia something mixed, what so you say to close these pr and from moatro latest main create a new one with only new unit teats for mutanta? |
Sure, if it suits you more. I can clean the PR from mixing changes and rebase it to the latest main. Whatever suits your needs |
sergey3bv
force-pushed
the
test/mut-dev-fee
branch
from
4956440 to
9ff8593
Compare
|
@sergey3bv tomorrow i will test back this, i promise! |
|
Good! @sergey3bv now just one file seems touched and i got: Found 28 mutants to test
ok Unmutated baseline in 118s build + 29s test
INFO Auto-set test timeout to 148s
28 mutants tested in 23m: 28 caught@grunch if you want to launch mostronator again... |
There was a problem hiding this comment.
Strict Review: PR #675 — Mutation testing for dev fee module
Single file changed: src/app/dev_fee.rs (229 additions, 25 deletions). The PR was correctly scoped down from the previous version — only mutation testing changes remain.
Previous Issues — Resolved
The previous review flagged unrelated changes (source tag removal, unused deps, docker/docs). Those were all removed from this PR. Only src/app/dev_fee.rs is modified. Well done.
Current Review
What Changed
cleanup_stale_pending_markersreturnsu32— count of reset orders (was void)- 6
#[mutants::skip]annotations — on LN-dependent functions that can't be unit tested - 11 new tests covering edge cases for atomic claims, timestamp parsing, stale cleanup, payment success, and LNURL invoice validation
- 1 existing test improved —
handle_payment_failurenow correctly setsdev_fee_paid = truebefore testing failure path
MEDIUM: Log level change is a regression
// BEFORE: only logs when stale_count > 0
if stale_count > 0 {
warn!("Reset {} stale PENDING dev fee orders (TTL: {}s)", stale_count, CLEANUP_TTL_SECS);
}
// AFTER: logs EVERY cycle, even when stale_count == 0
info!("Stale PENDING cleanup completed: reset {} orders (TTL: {}s)", stale_count, CLEANUP_TTL_SECS);This runs on every dev fee cycle. If DEV_FEE_CHECK_INTERVAL_SECS is short, this generates log noise. The info! level with reset 0 orders is useless — operators grep for non-zero resets.
Fix: Keep the conditional logging, or at minimum use debug! for zero resets:
if stale_count > 0 {
info!("Stale PENDING cleanup completed: reset {} orders (TTL: {}s)", stale_count, CLEANUP_TTL_SECS);
} else {
debug!("Stale PENDING cleanup: no stale orders found");
}
stale_countLOW: handle_payment_failure test setup improvement is correct but subtle
// BEFORE:
insert_test_order(&pool, order_id, "success", 100, false, Some("existing-hash")).await;
// AFTER:
insert_test_order(&pool, order_id, "success", 100, true, Some("existing-hash")).await;The true sets dev_fee_paid = 1 in the test order, which is correct — handle_payment_failure should reset a paid order back to unpaid. The old test was testing failure handling on an already-unpaid order, which was not a meaningful test.
This is a silent fix to an existing test bug. Consider adding a comment explaining why true is correct here.
All New Tests — Quality Assessment
| Test | Quality | Notes |
|---|---|---|
atomic_claim_fails_for_zero_dev_fee |
✅ Good | Edge case: zero fee should not be claimable |
atomic_claim_succeeds_for_settled_hold_invoice_status |
✅ Good | Tests settled-hold-invoice status path |
atomic_claim_fails_for_wrong_status |
✅ Good | active status should not be claimable |
test_parse_timestamp_at_threshold_boundary |
✅ Good | Exactly at 1B boundary returns None |
test_parse_timestamp_just_above_threshold |
✅ Good | 1B+1 returns Some |
test_parse_stripped_len_exactly_37 |
✅ Good | Boundary test for UUID length check |
test_parse_wrong_separator_at_position_36 |
✅ Good | Missing dash after UUID |
cleanup_stale_pending_markers_resets_legacy_marker |
✅ Good | Legacy format without timestamp |
cleanup_stale_pending_markers expanded to 2 stale |
✅ Good | Tests return value counting |
handle_payment_success_uses_lnd_hash_when_mismatch |
✅ Good | LND hash takes precedence over stored hash |
resolve_dev_fee_invoice_rejects_zero_fee |
✅ Good | Zero fee validation |
resolve_dev_fee_invoice_rejects_negative_fee |
✅ Good | Negative fee via manual DB update |
All tests are well-structured and test meaningful mutation-surviving paths.
#[mutants::skip] Annotations — Placement Assessment
| Function | Skip Justified? | Reason |
|---|---|---|
verify_confirmed_orders |
✅ Yes | Requires LN node |
recover_partial_payments |
✅ Yes | Requires LN node |
process_new_dev_fee_payments |
✅ Yes | Requires LN node + LNURL |
handle_payment_timeout |
✅ Yes | Requires LN node state |
check_dev_fee_payment_status |
✅ Yes | Requires LN node RPC |
send_dev_fee_payment |
✅ Yes | Requires LN node payment |
All skips are on functions that interact with external systems (LND). Correct placement.
Verdict
Request Changes — one issue:
- Fix log level: conditional
info!/debug!instead of unconditionalinfo!for every cycle
Once the log noise is fixed, this is ready to merge. The test coverage is thorough and well-targeted.
|
@sergey3bv the bot is almost happy! Good job! Fix the last details and we will merge... |
I am not sure, if it is really required. You see, there is a single |
No indeed, but maybe ( if not problematic for mutants ) the condition if_stale > 0 makes sense to avoid also printing logs when we don't have stale payments, do you agree? |
sergey3bv
force-pushed
the
test/mut-dev-fee
branch
from
9ff8593 to
fbd81e4
Compare
|
@sergey3bv please fix conflicts so we can merge it |
sergey3bv
force-pushed
the
test/mut-dev-fee
branch
from
fbd81e4 to
54f73bd
Compare
Done. Should I also merge a PR from @arkanoider? |
3 participants
Addressing #632
Summary by CodeRabbit