refactor: list methods return references #614
Conversation
chaitika
force-pushed
the
fix-list-methods-cloning
branch
2 times, most recently
from
dcaed45 to
592b197
Compare
chaitika
force-pushed
the
fix-list-methods-cloning
branch
2 times, most recently
from
659b22e to
8d2769a
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #614 +/- ##
==========================================
+ Coverage 68.87% 75.35% +6.47%
==========================================
Files 35 40 +5
Lines 4932 6480 +1548
==========================================
+ Hits 3397 4883 +1486
- Misses 1535 1597 +62 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
chaitika
force-pushed
the
fix-list-methods-cloning
branch
from
8d2769a to
668c963
Compare
| self.print_swap_report( | ||
| prereset_swapstate, | ||
| swap_start_time, | ||
| self.wallet.list_all_utxo(), | ||
| )?; |
There was a problem hiding this comment.
This won't work. The initial utxo set passed on here previously was the set that existed before the swap. This will pass the utxo set after the swap. They are different. So this will mess up the report logic.
Revert back to previous way of capturing the initial (pre-swap) utxo set.
| let (initial_outpoints, input_utxos): (HashSet<_>, Vec<_>) = initial_utxos | ||
| .into_iter() | ||
| .map(|utxo| { | ||
| let outpoint = OutPoint { |
There was a problem hiding this comment.
Try not to change logic not related to this PR, to reduce review surface. The previous way was just fine.
| } | ||
|
|
||
| /// Lists swap coin UTXOs along with their Spend info. | ||
| pub fn list_swap_coin_utxo_spend_info(&self) -> Vec<(ListUnspentResultEntry, UTXOSpendInfo)> { |
| /// Lists descriptor UTXOs along with their Spend info. | ||
| pub fn list_descriptor_utxo_spend_info(&self) -> Vec<(ListUnspentResultEntry, UTXOSpendInfo)> { |
| /// Lists all swept incoming swapcoin UTXOs along with their Spend info. | ||
| pub fn list_swept_incoming_swap_utxos(&self) -> Vec<(ListUnspentResultEntry, UTXOSpendInfo)> { |
| let coins = [(utxo.clone(), spend_info.clone())]; | ||
| let tx = self.spend_coins(&coins, destination, feerate)?; |
There was a problem hiding this comment.
This is not looking clean. We are cloning again which destroys the purpose of the PR.
Can we change spend_coin to take a reference too? In theory, it should work with refs. But it might need more refactoring changes internally.
| .map(|(utxo, _)| utxo.clone()) | ||
| .collect() | ||
| pub fn list_all_utxo(&self) -> impl Iterator<Item = &ListUnspentResultEntry> { | ||
| self.store.utxo_cache.values().map(|(utxo, _)| utxo) |
There was a problem hiding this comment.
better to use list_all_utxo_spend_info to avoid repeating change later.
| self.store | ||
| .utxo_cache | ||
| .values() |
There was a problem hiding this comment.
Same, use list_all_utxo_spend_info whenever possible.
If we keep the API pipeline same everywhere, then later if any changes needed, we can add it only to list_alll_utxo_spend_info without changing any other API.
| let coins = [(utxo.clone(), spend_info.clone())]; | ||
| let tx: Transaction = self.spend_coins(&coins, destination, feerate)?; |
There was a problem hiding this comment.
Same. Avoid cloning by changing spend_coins definitions.
4 participants
Fixes #604