Use dedicated struct for wallet balance #385

Levi0804 · 2025-01-22T12:37:13Z

This PR is a work in progress addressing #366.

mojoX911

Appraoch ACK. looks good. Few comments.

src/bin/maker-cli.rs

src/maker/rpc/messages.rs

src/maker/rpc/server.rs

mojoX911 · 2025-01-25T07:17:16Z

@Levi0804 can you rebase this on current master? Changs looks good. Once the conflicts are resolved it should be good to go.

Levi0804 · 2025-01-25T20:15:28Z

@mojoX911 I rebased this on master. Does this look good?

Levi0804 · 2025-01-25T21:39:56Z

In taker.rs, I think we can directly pretty print the balances inside the main function. What are your thoughts on this? Please let me know if you'd like me to make any changes.

mojoX911 · 2025-01-26T10:45:58Z

Code changes looks good.

In taker.rs, I think we can directly pretty print the balances inside the main function. What are your thoughts on this? Please let me know if you'd like me to make any changes.

Ack. But we can do more.

It would be even better if we add the Balances struct in the wallet::api itself. This will be useful for the wallet API users in downstream. Create a new function get_balances(&self) -> Balances. This function can be called wherever we need to get the any kind of balance.

This struct then also can be used in both taker and maker bins and there we pretty print it in json.

For taker the fidelity field will always be zero. So while printing we will just ommit that.

Put the Balances struct in the wallet/api.rs.

mojoX911 · 2025-01-26T10:57:01Z

This will also change the doc sections of balances for maker-cli and taker tutorials. https://github.com/citadel-tech/coinswap/tree/master/docs/app%20demos

lets do that in this PR itself.

You don't need to recreate the exact situation as the tutorial. Wherever we are showing balance outputs just replace with the new json format and put the previous values in respective fields.

KnowWhoami · 2025-01-26T11:15:41Z

@mojoX911 should we also create a similar structure for utxos?

mojoX911 · 2025-01-26T11:17:46Z

@mojoX911 should we also create a similar structure for utxos?

Yes. Also for fidelity bonds. Issues created, #387 #389

Levi0804 · 2025-01-28T05:54:48Z

This will also change the doc sections of balances for maker-cli and taker tutorials. https://github.com/citadel-tech/coinswap/tree/master/docs/app%20demos

lets do that in this PR itself.

You don't need to recreate the exact situation as the tutorial. Wherever we are showing balance outputs just replace with the new json format and put the previous values in respective fields.

Alright, I'll work on this next.

Levi0804 · 2025-01-28T05:56:42Z

Do the current changes look good to you?

mojoX911

Yes changes looks good. Awaiting CI.

mojoX911 · 2025-01-30T08:36:22Z

The maker-cli test is failing as the old commands are no longer valid. You will need to change the test commands and the output processing. Previously the output was a u32, now it's a json string. So it needs to be parsed correctly and then the individual values can be asserted.

coinswap/tests/maker_cli.rs

Lines 220 to 230 in 8cec81b

    
           // Check balances 
        
           assert_eq!(maker_cli.execute_maker_cli(&["get-balance"]), "999000 sats"); 
        
           assert_eq!( 
        
               maker_cli.execute_maker_cli(&["get-balance-contract"]), 
        
               "0 sats" 
        
           ); 
        
           assert_eq!( 
        
               maker_cli.execute_maker_cli(&["get-balance-fidelity"]), 
        
               "5000000 sats" 
        
           ); 
        
           assert_eq!(maker_cli.execute_maker_cli(&["get-balance-swap"]), "0 sats");

src/bin/maker-cli.rs

src/bin/taker.rs

Levi0804 · 2025-01-31T20:46:03Z

do these changes look good?

Levi0804 · 2025-01-31T20:47:17Z

This will also change the doc sections of balances for maker-cli and taker tutorials. https://github.com/citadel-tech/coinswap/tree/master/docs/app%20demos

lets do that in this PR itself.

You don't need to recreate the exact situation as the tutorial. Wherever we are showing balance outputs just replace with the new json format and put the previous values in respective fields.

coinswap/docs/app demos/maker-cli.md

Lines 215 to 225 in 64ed9f4

    
           ### CheckFidelityBalance 
        
           To check the balance of our fidelity UTXOs, use: 
        
           ```bash 
        
           $ ./maker-cli get-balance-fidelity 
        
           ``` 
        
           **Output:**   
        
           ```bash 
        
           50000 sats 
        
           ```

I should replace every balance command with get-balances, right?

mojoX911 · 2025-02-01T08:48:02Z

I should replace every balance command with get-balances, right?

Yes.

Appraoch ACK. Will do a deeper review soon.

Levi0804 · 2025-02-01T10:07:27Z

for some reason a check is still failing

codecov · 2025-02-01T12:13:35Z

Codecov Report

Attention: Patch coverage is 4.76190% with 20 lines in your changes missing coverage. Please review.

Project coverage is 70.50%. Comparing base (1f699c8) to head (77a9dbd).
Report is 15 commits behind head on master.

Files with missing lines	Patch %	Lines
src/maker/rpc/messages.rs	0.00%	8 Missing ⚠️
src/wallet/api.rs	12.50%	7 Missing ⚠️
src/bin/taker.rs	0.00%	2 Missing ⚠️
src/maker/rpc/server.rs	0.00%	2 Missing ⚠️
src/bin/maker-cli.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #385      +/-   ##
==========================================
- Coverage   70.52%   70.50%   -0.02%     
==========================================
  Files          34       34              
  Lines        4247     4242       -5     
==========================================
- Hits         2995     2991       -4     
+ Misses       1252     1251       -1

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

Levi0804 · 2025-02-02T19:45:45Z

We can now completely remove this, right?

coinswap/docs/app demos/maker-cli.md

Lines 272 to 343 in 64ed9f4

    
           >[!IMPORTANT] 
        
           > Currently `maker-cli` does not provide any method to see maker's normal wallet utxos and their correspinding balance seperately as we do have for other utxos types. 
        
           > we have to manually figure these utxos and their balances by using  `list-utxo` and `get-balance` command respectively. 
        
           > where `list-utxo` returns all the utxos present in the maker wallet including the `fidleity utxos` also. 
        
           > and `get-balance` returns the total spendable balance which includes balance of normal utxos , swap utxos, contract utxos but excludes `fidelitly utxos`. 
        
           Let's find them out:  
        
           ```bash 
        
            $ ./maker-cli  list-utxo 
        
             [ 
        
                 ListUnspentResultEntry { 
        
                     txid: 6c06a925066b0cf8adb400e53001b20587729407bce7dcb95dcacd038950b0e4, 
        
                     vout: 0, 
        
                     address: Some( 
        
                         Address<NetworkUnchecked>(BCRT1QKP92002WJPU5WFLU0YNTU3YXRVDR52N98STWFZJ7MF3F88RJ5PLQKDEUQY), 
        
                     ), 
        
                     label: Some( 
        
                         "ae28aba4", 
        
                     ), 
        
                     redeem_script: None, 
        
                     witness_script: None, 
        
                     script_pub_key: Script(OP_0 OP_PUSHBYTES_32 b04aa7bd4e90794727fc7926be44861b1a3a2a653c16e48a5eda62939c72a07e), 
        
                     amount: 50000 SAT, 
        
                     confirmations: 1, 
        
                     spendable: true, 
        
                     solvable: false, 
        
                     descriptor: None, 
        
                     safe: true, 
        
                 }, 
        
                 ListUnspentResultEntry { 
        
                     txid: 6c06a925066b0cf8adb400e53001b20587729407bce7dcb95dcacd038950b0e4, 
        
                     vout: 1, 
        
                     address: Some( 
        
                         Address<NetworkUnchecked>(BCRT1QC538UUY77TN2YLYPTLXEQ6S8GL55753UK9C909), 
        
                     ), 
        
                     label: None, 
        
                     redeem_script: None, 
        
                     witness_script: None, 
        
                     script_pub_key: Script(OP_0 OP_PUSHBYTES_20 c5227e709ef2e6a27c815fcd906a0747e94f523c), 
        
                     amount: 949000 SAT, 
        
                     confirmations: 1, 
        
                     spendable: true, 
        
                     solvable: true, 
        
                     descriptor: Some( 
        
                         "wpkh([bd63c57a/1/0]024974169b3f59a123ac00e5034edd256593204cfab5668e5751d42bc864e0e955)#ljsywwyv", 
        
                     ), 
        
                     safe: true, 
        
                 }, 
        
             ] 
        
           ```   
        
           We created a funding transaction to fund the maker wallet and establish the fidelity bonds. As a result, the command displays two UTXOs:  
        
           1. The **fidelity UTXO** (which we've already seen). 
        
           2. The **normal funding UTXO**. 
        
           ### Breakdown: 
        
           - Initially, we funded the wallet with `0.01 BTC`. 
        
           - `50,000 sats` were used for the fidelity bond. 
        
           - `1,000 sats` were used as the mining fee for the fidelity transaction. 
        
           The remaining balance after these transactions is: 
        
           **949,000 sats** = **1,000,000 sats** (total funding) - **50,000 sats** (for the fidelity bond) - **1,000 sats** (mining fees). 
        
           We can verify this balance by running the `get-balance` command, which shows the total spendable balance (excluding the fidelity bond balance): 
        
           ```bash 
        
           $ ./maker-cli get-balance 
        
           949,000 sats 
        
           ```

mojoX911 · 2025-02-03T05:06:51Z

We can now completely remove this, right?

Only line 273. Rest are fine.

mojoX911

Ack. Code looks good. Few nits and suggets.

docs/app demos/taker.md

mojoX911 · 2025-02-03T08:26:04Z

tests/abort1.rs

-            let live_contract_balance = wallet.balance_live_contract(Some(&all_utxos)).unwrap();
+            let live_contract_balance = wallet
+                .balance_live_timelock_contract(Some(&all_utxos))
+                .unwrap();


We now don't need to fetch individual balances. We can call get_balances and assert values in the balance structure. This will simplify the code, as well as put get_balances in the coverage.

Same changes will be applicable to all tests and mod.rs also. This can be done as a followup PR.

tests/taker_cli.rs

mojoX911 · 2025-02-03T08:29:25Z

tests/test_framework/mod.rs

-    let live_contract_balance = wallet.balance_live_contract(Some(&all_utxos)).unwrap();
+    let live_contract_balance = wallet
+        .balance_live_timelock_contract(Some(&all_utxos))
+        .unwrap();


Same thing here also, they can all be combined in one call.

Levi0804 · 2025-02-03T10:38:52Z

Do these changes look good?
I'll address the other requested changes in a separate PR as you suggested.

mojoX911

Ack

Levi0804 mentioned this pull request Jan 22, 2025

Use dedicated struct for wallet balance. #366

Open

This comment was marked as spam.

Sign in to view

mojoX911 requested changes Jan 22, 2025

View reviewed changes

src/bin/maker-cli.rs Outdated Show resolved Hide resolved

src/maker/rpc/messages.rs Outdated Show resolved Hide resolved

src/maker/rpc/server.rs Outdated Show resolved Hide resolved

Levi0804 requested a review from mojoX911 January 24, 2025 19:02

Levi0804 added 2 commits January 26, 2025 01:32

add dedicated struct for utxo balances

1852f05

address review comments

aa7770f

Levi0804 force-pushed the dedicated-struct branch from 69df8ed to aa7770f Compare January 25, 2025 20:08

enhance(taker.rs): pretty print total wallet balances by category

9c5f7e4

add Balances struct to wallet::api

fe296e2

mojoX911 reviewed Jan 29, 2025

View reviewed changes

mojoX911 requested changes Jan 30, 2025

View reviewed changes

src/bin/maker-cli.rs Outdated Show resolved Hide resolved

src/bin/taker.rs Outdated Show resolved Hide resolved

fix failing tests

aff133d

rename Balance to Balances

bb9523a

update maker cli and taker tutorials

6414699

mojoX911 requested changes Feb 3, 2025

View reviewed changes

address review comments

4637216

Levi0804 changed the title ~~Use Dedicated struct for wallet balance~~ Use dedicated struct for wallet balance Feb 3, 2025

remove println import

77a9dbd

mojoX911 approved these changes Feb 3, 2025

View reviewed changes

mojoX911 merged commit 2097858 into citadel-tech:master Feb 3, 2025
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use dedicated struct for wallet balance #385

Use dedicated struct for wallet balance #385

Levi0804 commented Jan 22, 2025

This comment was marked as spam.

mojoX911 left a comment

mojoX911 commented Jan 25, 2025

Levi0804 commented Jan 25, 2025

Levi0804 commented Jan 25, 2025

mojoX911 commented Jan 26, 2025 •

edited

Loading

mojoX911 commented Jan 26, 2025 •

edited

Loading

KnowWhoami commented Jan 26, 2025

mojoX911 commented Jan 26, 2025

Levi0804 commented Jan 28, 2025

Levi0804 commented Jan 28, 2025

mojoX911 left a comment

mojoX911 commented Jan 30, 2025 •

edited

Loading

Levi0804 commented Jan 31, 2025

Levi0804 commented Jan 31, 2025

mojoX911 commented Feb 1, 2025

Levi0804 commented Feb 1, 2025

codecov bot commented Feb 1, 2025 •

edited

Loading

Levi0804 commented Feb 2, 2025

mojoX911 commented Feb 3, 2025

mojoX911 left a comment

mojoX911 Feb 3, 2025

mojoX911 Feb 3, 2025

Levi0804 commented Feb 3, 2025

mojoX911 left a comment

Use dedicated struct for wallet balance #385

Use dedicated struct for wallet balance #385

Conversation

Levi0804 commented Jan 22, 2025

This comment was marked as spam.

mojoX911 left a comment

Choose a reason for hiding this comment

mojoX911 commented Jan 25, 2025

Levi0804 commented Jan 25, 2025

Levi0804 commented Jan 25, 2025

mojoX911 commented Jan 26, 2025 • edited Loading

mojoX911 commented Jan 26, 2025 • edited Loading

KnowWhoami commented Jan 26, 2025

mojoX911 commented Jan 26, 2025

Levi0804 commented Jan 28, 2025

Levi0804 commented Jan 28, 2025

mojoX911 left a comment

Choose a reason for hiding this comment

mojoX911 commented Jan 30, 2025 • edited Loading

Levi0804 commented Jan 31, 2025

Levi0804 commented Jan 31, 2025

mojoX911 commented Feb 1, 2025

Levi0804 commented Feb 1, 2025

codecov bot commented Feb 1, 2025 • edited Loading

Codecov Report

Levi0804 commented Feb 2, 2025

mojoX911 commented Feb 3, 2025

mojoX911 left a comment

Choose a reason for hiding this comment

mojoX911 Feb 3, 2025

Choose a reason for hiding this comment

mojoX911 Feb 3, 2025

Choose a reason for hiding this comment

Levi0804 commented Feb 3, 2025

mojoX911 left a comment

Choose a reason for hiding this comment

mojoX911 commented Jan 26, 2025 •

edited

Loading

mojoX911 commented Jan 26, 2025 •

edited

Loading

mojoX911 commented Jan 30, 2025 •

edited

Loading

codecov bot commented Feb 1, 2025 •

edited

Loading