Skip to content

Conversation

@0xEgao
Copy link
Collaborator

@0xEgao 0xEgao commented Dec 8, 2025

This PR works on adding the remaining edge cases to test to ensure the Taproot-based Coinswap protocol works effectively and safely under both normal and failure conditions.

Works on issue #658

Already Covered Test Cases

  • Two maker single taker swap
  • Maker abort after SenderContract from Taker
  • Intermediate maker abort after SenderContractFromMaker
  • Final maker abort after SenderContractFromMaker
  • Incoming contract timelock broadcast before PrivateKeyHandover
  • Outgoing contract hashlock broadcast before PrivateKeyHandover

Added in This PR

  • Multi maker single taker swap
  • Multi taker multi maker swap
  • Not enough makers
  • Maker abort after AckResponse on SwapDetails
  • Maker abort after PrivateKeyHandover by Taker
  • Taker abort after AckResponse from Maker
  • Taker abort after SenderContract from Taker
  • Taker abort after SenderContractFromMaker

@codecov
Copy link

codecov bot commented Dec 9, 2025

Codecov Report

❌ Patch coverage is 96.70330% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.76%. Comparing base (776b75d) to head (36bf352).
⚠️ Report is 71 commits behind head on master.

Files with missing lines Patch % Lines
src/bin/taker.rs 0.00% 2 Missing ⚠️
src/taker/api2.rs 98.46% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #649      +/-   ##
==========================================
+ Coverage   68.87%   77.76%   +8.88%     
==========================================
  Files          35       49      +14     
  Lines        4932    15126   +10194     
==========================================
+ Hits         3397    11762    +8365     
- Misses       1535     3364    +1829     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@0xEgao 0xEgao marked this pull request as ready for review December 20, 2025 19:50
@0xEgao 0xEgao force-pushed the taproot-test branch 2 times, most recently from 5bac6d5 to 14a4c84 Compare December 23, 2025 19:36
Copy link

@mojoX911 mojoX911 left a comment

Choose a reason for hiding this comment

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

Overall Ack. Just one major comment for all the test cases.

Instead of asserting for a range (< or >) can we assert for exact values? For all swaps that failed, succeeded, or recovered, the difference between balances should always be a fixed constant number. So we can assert for that number directly, and also mention in a comment why this difference in balance is occurring.

Currently, we are not able to see how much exactly the takers and makers are losing in each scenario.

@0xEgao
Copy link
Collaborator Author

0xEgao commented Dec 26, 2025

Overall Ack. Just one major comment for all the test cases.

Instead of asserting for a range (< or >) can we assert for exact values? For all swaps that failed, succeeded, or recovered, the difference between balances should always be a fixed constant number. So we can assert for that number directly, and also mention in a comment why this difference in balance is occurring.

Currently, we are not able to see how much exactly the takers and makers are losing in each scenario.

Done

@0xEgao
Copy link
Collaborator Author

0xEgao commented Dec 27, 2025

Have used assert_in_range macro for maker spendable balance check due to the sporadic cases of +-2sats happening.
No such issues with taker balance check's observed.

@mojoX911
Copy link

@0xEgao I think we should also assert for banning behaviors in the taproot tests, just as we do it for V1 tests also.

info!("🚫 Verifying naughty maker gets banned");
    // Maker gets banned for being naughty.
    assert_eq!(
        format!("127.0.0.1:{naughty}"),
        taker.get_bad_makers()[0].address.to_string()
    );

@0xEgao
Copy link
Collaborator Author

0xEgao commented Dec 30, 2025

Have made the suggested changes i.e. :
1-) Log swap balance for takers
2-) Better balance checks && comments
3-) Mark bad makers
4-) remove saturated_sub

@0xEgao 0xEgao force-pushed the taproot-test branch 2 times, most recently from 6bf12dd to 087c70b Compare December 30, 2025 16:37
@mojoX911
Copy link

mojoX911 commented Jan 2, 2026

@stark-3k can you mark the following TODOs in the tests:

  • There are certain fee variances coming for various swaps. Mark a todo to investigate why.
  • In case of maker recovery, we are waiting for a timeout. Instead, we should check for the correct log and wait till the maker recovers. This will need some fixes as the current log checking API doesn't handle macOS.

We will turn these todos into issues later, after release. For now this is good to go.

Copy link

@mojoX911 mojoX911 left a comment

Choose a reason for hiding this comment

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

Ack

@mojoX911 mojoX911 merged commit 9ce1421 into citadel-tech:master Jan 2, 2026
9 checks passed
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.

[Taproot] Checklist for protocol testing [TAPROOT] Cover Edge Test Cases

3 participants