WalletSwapCoin: Split into two classes + add trait #29

GeneFerneau · 2021-07-30T18:10:08Z

Convert WalletSwapCoin into Incoming/OutgoingSwapCoin, and add a WalletSwapCoin trait for common functionality

codecov-commenter · 2021-07-30T18:27:34Z

Codecov Report

Merging #29 (e150537) into master (1d9b34b) will decrease coverage by 2.90%.
The diff coverage is 47.03%.

@@            Coverage Diff             @@
##           master      #29      +/-   ##
==========================================
- Coverage   80.83%   77.92%   -2.91%     
==========================================
  Files           8        8              
  Lines        2588     2750     +162     
==========================================
+ Hits         2092     2143      +51     
- Misses        496      607     +111

Impacted Files	Coverage Δ
src/main.rs	`43.49% <0.00%> (+0.41%)`	⬆️
src/wallet_sync.rs	`72.28% <42.48%> (-6.05%)`	⬇️
src/contracts.rs	`85.08% <54.38%> (-6.92%)`	⬇️
src/maker_protocol.rs	`79.04% <63.63%> (-1.79%)`	⬇️
src/messages.rs	`86.90% <100.00%> (ø)`
src/taker_protocol.rs	`95.07% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d9b34b...e150537. Read the comment docs.

chris-belcher · 2021-07-31T06:44:25Z

Thanks for the PR(!)

chris-belcher · 2021-07-31T12:56:27Z

src/contracts.rs

+    fn get_other_privkey(&self) -> Option<&SecretKey>;
+    fn get_contract_privkey(&self) -> Option<&SecretKey>;
+    fn get_hash_preimage(&self) -> Option<&[u8; 32]>;


These functions should not be part of the trait SwapCoin.

Because WatchOnlySwapCoins will never have access to these, and our code ends up trying to get the hash preimage or contract privkey from a WatchOnlySwapCoin then the compiler should raise an error. I got the idea from this section https://doc.rust-lang.org/book/ch17-03-oo-design-patterns.html#encoding-states-and-behavior-as-types

My other comments right now will explain how the code can be modified to never require these three functions in the SwapCoin trait

I removed these functions from SwapCoin. Added a couple other functions to get the contract type, and whether contract information is known (other_privkey for IncomingSwapCoin, hash_preimage for Incoming/OutgoingSwapCoin), and the type string.

src/wallet_sync.rs

chris-belcher · 2021-07-31T13:02:52Z

src/wallet_sync.rs

+        for (multisig_redeemscript, swapcoin) in &self.incoming_swap_coins {
+            Self::print_script_and_coin(multisig_redeemscript, swapcoin);
+        }
+        for (multisig_redeemscript, swapcoin) in &self.outgoing_swap_coins {
+            Self::print_script_and_coin(multisig_redeemscript, swapcoin);
        }
-        println!("swapcoin count = {}", self.swap_coins.len());
+        println!(
+            "swapcoin count = {}",
+            self.incoming_swap_coins.len() + self.outgoing_swap_coins.len()
+        );
+    }
+
+    fn print_script_and_coin(script: &Script, coin: &dyn SwapCoin) {
+        let contract_tx = coin.get_contract_tx();
+        println!(
+            "{} {}:{} {}",
+            Address::p2wsh(script, NETWORK),
+            contract_tx.input[0].previous_output.txid,
+            contract_tx.input[0].previous_output.vout,
+            if coin.get_other_privkey().is_some() {
+                "  known"
+            } else {
+                "unknown"
+            }
+        )


We can avoid using SwapCoin.get_other_privkey() here if we retain knowledge of the type (either IncomingSwapCoin or OutgoingSwapCoin). OutgoingSwapCoin will never have the field other_privkey. Probably this function print_script_and_coin should only take IncomingSwapCoins.

Probably this function print_script_and_coin should only take IncomingSwapCoins.

I think it's important to print all the coins in this context, and added an is_known function to SwapCoin. For IncomingSwapCoin is tests the presence of hash_preimage or other_privkey, and for OutgoingSwapCoin only tests for hash_preimage.

chris-belcher · 2021-07-31T13:04:44Z

src/main.rs

                    compressed: true,
                    key: bitcoin::secp256k1::PublicKey::from_secret_key(
                        &secp,
-                        &swapcoin.contract_privkey,
+                        swapcoin.get_contract_privkey().unwrap(),
                    ),
                };
                let type_string = if contract_pubkey
-                    == read_hashlock_pubkey_from_contract(&swapcoin.contract_redeemscript).unwrap()
+                    == read_hashlock_pubkey_from_contract(&swapcoin.get_contract_redeemscript())
+                        .unwrap()
                {
                    "hashlock"
                } else {
                    assert_eq!(
                        contract_pubkey,
-                        read_timelock_pubkey_from_contract(&swapcoin.contract_redeemscript)
+                        read_timelock_pubkey_from_contract(&swapcoin.get_contract_redeemscript())
                            .unwrap()
                    );
                    "timelock"


As "timelock" and "hashlock" exactly match the OutgoingSwapCoin and IncomingSwapCoin types, we no longer need to do this whole part where we calculate the public key and compare it with the contract_redeemscript. Consequently we also don't need the function get_contract_privkey.

I replaced these tests with a function SwapCoin::contract_type that directly returns the type string:

IncomingSwapCoin => "hashlock"

OutgoingSwapCoin => "timelock"

WatchOnlySwapCoin => "watchonly"

chris-belcher · 2021-07-31T13:14:50Z

I've done a first-pass review and written some comments. They're actually all related to one issue about not having three functions be in the SwapCoin trait.

There's also another issue about how in maker_protocol.rs its wrong to check both the get_incoming_swapcoin and get_outgoing_swapcoin functions, however that can be fixed later it doesn't have to be in this PR.

GeneFerneau · 2021-08-02T19:37:29Z

There's also another issue about how in maker_protocol.rs its wrong to check both the get_incoming_swapcoin and get_outgoing_swapcoin functions, however that can be fixed later it doesn't have to be in this PR.

I was wondering if there was only a need to check for IncomingSwapCoin or OutgoingSwapCoin in handle_sign_receivers_contract_tx, but didn't have enough context to make the call. So, defaulted to checking for both types, since that's what the previous functionality did.

I've added a fixup commit, and rebased on latest master. Let me know what you think. Will squash the fixup commit(s) once you're happy with the changes.

chris-belcher · 2021-08-19T09:25:59Z

src/wallet_sync.rs

@@ -604,20 +786,25 @@ impl Wallet {
    pub fn find_incomplete_coinswaps(
        &self,
        rpc: &Client,
-    ) -> Result<HashMap<[u8; 20], Vec<(ListUnspentResultEntry, &WalletSwapCoin)>>, Error> {
+    ) -> Result<HashMap<[u8; 20], Vec<(ListUnspentResultEntry, &dyn SwapCoin)>>, Error> {


Please change this whole function a little bit to return HashMap::<[u8; 20]>, (Vec<(ListUnspentResultEntry, &IncomingSwapCoin)>, Vec<(ListUnspentResultEntry, &OutgoingSwapCoin)>). So make the IncomingSwapCoin and OutgoingSwapCoin be returned separately in different vectors.

chris-belcher · 2021-08-19T09:28:13Z

Sorry about the delay on my end. Thanks for sticking with this.

Just one requested change now, and after that please squish the commits. I'll have a bit of time right now if you reckon you'll be delayed I can just merge this PR and do the requested change myself.

GeneFerneau · 2021-08-19T20:06:24Z

Sorry about the delay on my end. Thanks for sticking with this.

No worries, thanks for the review!

Just one requested change now, and after that please squish the commits.

~~Will do~~ mostly done

Rebased on latest master, and added the requested change. Let me know if you like it, or want me to revert. Makes the code a little more verbose, but has the advantage of concrete types in a couple places.

If you like it, I'll squish the commits.

src/main.rs

chris-belcher · 2021-08-20T11:26:00Z

I just tested if wallet-balance could show incomplete coinswaps. I started a maker and then did coinswap-send with the taker, then after the first message waiting for funding transaction to confirm I closed the maker and taker, and told regtest to generate a block (to confirm the funding transaction). But now running wallet-balance on the taker wallet doesn't show any incomplete coinswap but it should.

GeneFerneau · 2021-08-21T05:54:27Z

I just tested if wallet-balance could show incomplete coinswaps. I started a maker and then did coinswap-send with the taker, then after the first message waiting for funding transaction to confirm I closed the maker and taker, and told regtest to generate a block (to confirm the funding transaction). But now running wallet-balance on the taker wallet doesn't show any incomplete coinswap but it should.

Hopefully, the fix for the iterators in main, fix the reporting issue. If not, let me know, and I'll try to reproduce your test setup.

Sidenote: should I work on setting up automated integration tests for situations like the one you mentioned?

src/wallet_sync.rs

chris-belcher · 2021-08-24T09:21:20Z

That last commit did not fix the issue. So I just put print statements everywhere myself and I tracked down the cause, which I wrote in a review comment here just now.

Sidenote: Yes but not right now. I'm still working on code which will go alongside this PR which waits for the timeout branch of the contract transactions and then adds those coins to the wallet.

Convert WalletSwapCoin into Incoming/OutgoingSwapCoin, and add a WalletSwapCoin trait for common functionality

GeneFerneau force-pushed the swapcoin branch from 3bf208a to 3d67620 Compare July 30, 2021 18:20

chris-belcher reviewed Jul 31, 2021

View reviewed changes

src/wallet_sync.rs Outdated Show resolved Hide resolved

chris-belcher reviewed Jul 31, 2021

View reviewed changes

GeneFerneau force-pushed the swapcoin branch from 950c495 to f33e6bd Compare August 2, 2021 19:36

GeneFerneau force-pushed the swapcoin branch 3 times, most recently from a3f584f to 06b00be Compare August 2, 2021 19:53

chris-belcher reviewed Aug 19, 2021

View reviewed changes

GeneFerneau force-pushed the swapcoin branch from 06b00be to ee48eae Compare August 19, 2021 23:27

chris-belcher reviewed Aug 20, 2021

View reviewed changes

src/main.rs Outdated Show resolved Hide resolved

GeneFerneau force-pushed the swapcoin branch 2 times, most recently from 4b39730 to 2b7b188 Compare August 21, 2021 05:51

chris-belcher reviewed Aug 24, 2021

View reviewed changes

src/wallet_sync.rs Outdated Show resolved Hide resolved

chris-belcher reviewed Aug 24, 2021

View reviewed changes

src/wallet_sync.rs Outdated Show resolved Hide resolved

chris-belcher reviewed Aug 24, 2021

View reviewed changes

src/wallet_sync.rs Outdated Show resolved Hide resolved

WalletSwapCoin: Split into two classes + add trait

e150537

Convert WalletSwapCoin into Incoming/OutgoingSwapCoin, and add a WalletSwapCoin trait for common functionality

GeneFerneau force-pushed the swapcoin branch from 2b7b188 to e150537 Compare August 25, 2021 01:46

chris-belcher merged commit 5a4bf1c into bitcoin-teleport:master Aug 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WalletSwapCoin: Split into two classes + add trait #29

WalletSwapCoin: Split into two classes + add trait #29

GeneFerneau commented Jul 30, 2021

codecov-commenter commented Jul 30, 2021 •

edited

Loading

chris-belcher commented Jul 31, 2021

chris-belcher Jul 31, 2021

GeneFerneau Aug 2, 2021

chris-belcher Jul 31, 2021

GeneFerneau Aug 2, 2021

chris-belcher Jul 31, 2021

GeneFerneau Aug 2, 2021

chris-belcher commented Jul 31, 2021

GeneFerneau commented Aug 2, 2021

chris-belcher Aug 19, 2021

chris-belcher commented Aug 19, 2021

GeneFerneau commented Aug 19, 2021 •

edited

Loading

chris-belcher commented Aug 20, 2021

GeneFerneau commented Aug 21, 2021

chris-belcher commented Aug 24, 2021 •

edited

Loading

WalletSwapCoin: Split into two classes + add trait #29

WalletSwapCoin: Split into two classes + add trait #29

Conversation

GeneFerneau commented Jul 30, 2021

codecov-commenter commented Jul 30, 2021 • edited Loading

Codecov Report

chris-belcher commented Jul 31, 2021

chris-belcher Jul 31, 2021

Choose a reason for hiding this comment

GeneFerneau Aug 2, 2021

Choose a reason for hiding this comment

chris-belcher Jul 31, 2021

Choose a reason for hiding this comment

GeneFerneau Aug 2, 2021

Choose a reason for hiding this comment

chris-belcher Jul 31, 2021

Choose a reason for hiding this comment

GeneFerneau Aug 2, 2021

Choose a reason for hiding this comment

chris-belcher commented Jul 31, 2021

GeneFerneau commented Aug 2, 2021

chris-belcher Aug 19, 2021

Choose a reason for hiding this comment

chris-belcher commented Aug 19, 2021

GeneFerneau commented Aug 19, 2021 • edited Loading

chris-belcher commented Aug 20, 2021

GeneFerneau commented Aug 21, 2021

chris-belcher commented Aug 24, 2021 • edited Loading

codecov-commenter commented Jul 30, 2021 •

edited

Loading

GeneFerneau commented Aug 19, 2021 •

edited

Loading

chris-belcher commented Aug 24, 2021 •

edited

Loading