fix(p2pk): show P2PK balance by mariocynicys · Pull Request #2053 · GLEECBTC/komodo-defi-framework

mariocynicys · 2024-01-18T16:06:11Z

This makes it so we don't auto-convert P2PK addresses to P2PKH addresses, thus we can query them for balances if they have any.

Also refactors utxo::output_script() to auto detect the address type without the need for a keys::Type flag.

Fixes #720

mm2src/mm2_bitcoin/keys/src/lib.rs

mariocynicys · 2024-01-23T15:50:58Z

Missing unit tests for the balance addition. Verified it's working as expected with manual tests though.

artemii235

Docker tests fail to compile with these changes, they can be run using
cargo test --test 'docker_tests_main' --features run-docker-tests

Though, some of them are known to fail, which is partially fixed in another PR.

cc @shamardy

mm2src/coins/utxo.rs

shamardy · 2024-02-02T02:21:02Z

@mariocynicys can you please fix failing docker tests compilation by running this command cargo test --test 'docker_tests_main' --features run-docker-tests and checking the errors. You can wait for #1960 to be merged first but don't forget to check these errors after that.

shamardy

Great start! A few comments/notes for now but will do a complete review after #1960 is merged and conflicts are resolved.

mm2src/coins/utxo/rpc_clients.rs

mm2src/mm2_bitcoin/keys/src/lib.rs

shamardy · 2024-02-02T09:29:40Z

mm2src/coins/utxo/rpc_clients.rs

+            Ok(this
+                .scripthash_get_balances(hashes)
+                .compat()
+                .await?
+                .into_iter()
+                .fold(BigDecimal::from(0), |sum, electrum_balance| {
+                    sum + electrum_balance.to_big_decimal(decimals)
+                }))


It's a bit of an overhead to send 2 requests to electrums to get both P2PK and P2PKH balances when most addresses will not have P2PK outputs but I don't see a way around it. Just opening this for discussions in case you or anybody else has a better idea.
P.S. we opted for this solution as any other solution we could find would lead to a new coin for P2PK addresses, some explorers do the same as it was pointed out in a note in the code changes but others don't. A complete solution would be to have only segwit addresses for any coin that supports it and include all kind of outputs in the balance, but this is outside the scope of this issue/PR and more related to this #2050. This solution would lead to having balance on addresses completely different from explorers though.

shamardy · 2024-02-02T09:34:15Z

@mariocynicys As discussed, we should also implement P2PK outputs spending in this PR. You should wait for the merge of the conflicting PR first for this too.

This adds a new `.pubkey` field `Address` of type `Option<Public>`. Using this, we have a way to keep the pubkey of our address so that we can pull p2pk balance if any.

mariocynicys · 2024-02-12T14:17:12Z

The changes made up till now (showing p2pk balance) are reviewable. p2pk spending isn't added yet.

update: removed the under review status so the CI doesn't complain, still reviewable though.

instead of having one output script per address. This facilitates spending outputs of different types in the same transaction (e.g. spending p2pk outputs along with p2pkh outputs in the same transaction. note that p2pk output script is different than p2pkh one)

onur-ozkan · 2024-02-20T12:34:52Z

mm2src/coins/utxo/rpc_clients.rs

+        let output_script = try_f!(output_script(address));
+        let mut output_scripts = vec![output_script];


nit:

Suggested change

let output_script = try_f!(output_script(address));

let mut output_scripts = vec![output_script];

let mut output_scripts = vec![try_f!(output_script(address))];

onur-ozkan · 2024-02-20T12:36:13Z

mm2src/coins/utxo/slp.rs

            outputs,
        };
        Ok((preimage, recently_spent))
+        //////////////////////////////////////////////////


I guess a leftover

onur-ozkan · 2024-02-20T12:44:43Z

mm2src/mm2_bitcoin/script/src/script.rs

                    _ => unreachable!(), // because we are relying on script_type() checks here
                })
                .map(|public| {
+                    // DISCUSS: Should we really reduce this to pkh? what is it used for?


If we will merge this note, it would be great if we can change this to TODO. I often grep this placeholder in the entire codebase to see what we left behind.

Suggested change

// DISCUSS: Should we really reduce this to pkh? what is it used for?

// TODO: Discussion needed: "Should we really reduce this to pkh? what is it used for?"

I intend to get this resolved before the merger of the PR.

Should we really reduce this to pkh?

btw you can check the original ExtractDestination in the daemon code, I can see it works in the similar way as basically used to get the address from P2PK utxos, to add the utxo to the wallet balance

btw you can check the original ExtractDestination in the daemon code, I can see it works in the similar way as basically used to get the address from P2PK utxos, to add the utxo to the wallet balance

iirc, yeah, it's used somewhere to extract the address the coins are sent to, but they are just used for comparison and not stored anymore (basically, is this output sent our address?). the optional pubkey field doesn't contribute to that comparison (see).

maybe we can check if this output belongs to us by comparing output scripts instead?

onur-ozkan · 2024-02-20T12:47:23Z

mm2src/coins/utxo/utxo_common.rs

+    // let signature_version = match prev_script.is_pay_to_witness_key_hash() {
+    //     true => SignatureVersion::WitnessV0,
+    //     _ => coin.as_ref().conf.signature_version,
+    // };


Is this a leftover or needed for future work/ref? If we are going to merge this part too, we need some notes explaining why we keeping this.

onur-ozkan · 2024-02-20T12:53:57Z

mm2src/coins/utxo_signer/src/with_key_pair.rs

 ) -> UtxoSignWithKeyPairResult<TransactionInput> {
    let unsigned_input = get_input(signer, input_index)?;

+    // DISCUSS: Why are we doing this check? Solely to check whether we are spending


Same (about the TODO thing) here

…ctionInput` instead This allows us to get rid of the assumingly never triggered `UtxoSignWithKeyPairError::InputIndexOutOfBound` error. `mm2src/mm2_bitcoin/script/src/sign.rs` will need a similar refactor since it still uses indices and not unsigned inputs directly.

…edTransactionInput` instead" Revert the above commit since it introduces panics (`Vec[index]` this indexing technique might panic!). This reverts commit 926bc46.

…put is not used

shamardy

Next review iteration!
@mariocynicys please note that the below tests are failing here and not in dev, after fixing those I will do the final review round :)

mm2_tests::mm2_tests_inner::test_sign_raw_transaction_p2wpkh
mm2_tests::mm2_tests_inner::test_sign_raw_transaction_rick

@dimxy please resolve any comments that were fixed!

mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs

mm2src/mm2_bitcoin/script/src/sign.rs

shamardy · 2024-04-22T21:49:58Z

mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs

        conf.bech32_hrp.clone(),
    )
-    .as_pkh()
+    .as_pkh_from_pk(*key_pair.public())


Is this done? should HD PRs be merged first?

shamardy · 2024-05-06T14:56:46Z

@mariocynicys please fix conflicts in this PR so that we can review it.

mariocynicys · 2024-05-08T10:56:53Z

@shamardy, sorry, missed the last suggestions. Done now.

There seems to be some of this pattern in this test file though. Let me try to handle these as well in this PR.

shamardy

Thanks a lot for your patience during this PR review! Only non-blockers left!
@dimxy please check if all your comments are resolved so that I can merge this.

shamardy · 2024-05-08T10:44:18Z

mm2src/coins/utxo/utxo_withdraw.rs

+        // Do we want to mix P2PK and non-P2PK spends?
+        // Should we make another sweep P2PK method that spends all P2PK balance?


Let's not do that in this PR. Please open an issue for this so that we can consider it later.

Oops, forgot about this one, will put fixmes next time.

The logic right now mixes p2pk and non-p2pk (e.g. p2pkh, p2wpkh) and signs each input accordingly. So we should get rid of this comment.

shamardy · 2024-05-08T11:09:24Z

@smk762 if you have time, can you please check if your comment here #720 (comment) is resolved? If I merged it before that, you can check this in dev :)

by adding -Segwit to coin name

dimxy

LGTM

* dev: feat(ETH): eip1559 gas fee estimator and rpcs (#2051) fix(p2pk-tests): fix p2pk tests post merge (#2119) fix(p2pk): show and spend P2PK balance (#2053) fix(swap): use tmp file for swap and order files (#2118)

shamardy added the in progress label Jan 18, 2024

mariocynicys changed the title ~~[wip] Fix: Show P2PK balance~~ [wip] fix(p2pk): Show P2PK balance Jan 18, 2024

mariocynicys marked this pull request as draft January 18, 2024 16:21

mariocynicys changed the title ~~[wip] fix(p2pk): Show P2PK balance~~ fix(p2pk): Show P2PK balance Jan 18, 2024

shamardy changed the title ~~fix(p2pk): Show P2PK balance~~ fix(p2pk): Show P2PK balance Jan 18, 2024

shamardy changed the title ~~fix(p2pk): Show P2PK balance~~ fix(p2pk): show P2PK balance Jan 18, 2024

mariocynicys commented Jan 21, 2024

View reviewed changes

mm2src/mm2_bitcoin/keys/src/lib.rs Outdated Show resolved Hide resolved

mariocynicys marked this pull request as ready for review January 23, 2024 15:51

artemii235 reviewed Jan 29, 2024

View reviewed changes

mm2src/coins/utxo.rs Outdated Show resolved Hide resolved

shamardy mentioned this pull request Feb 1, 2024

refactor(utxo): refactor utxo output script creation #1960

Merged

shamardy reviewed Feb 2, 2024

View reviewed changes

mariocynicys force-pushed the p2pk-balances branch from ccd6147 to 0a24680 Compare February 8, 2024 17:03

mariocynicys added 2 commits February 9, 2024 00:43

fix copyright format

50b4aad

account for p2pk balances

ce267a9

This adds a new `.pubkey` field `Address` of type `Option<Public>`. Using this, we have a way to keep the pubkey of our address so that we can pull p2pk balance if any.

mariocynicys force-pushed the p2pk-balances branch from 0a24680 to ce267a9 Compare February 9, 2024 06:11

mariocynicys added the under review label Feb 12, 2024

mariocynicys removed the under review label Feb 12, 2024

mariocynicys added 4 commits February 13, 2024 18:50

fix docker tests

b70393c

fix lint issues (unused vars)

9ccc118

allow spending p2pk balance

0086b41

onur-ozkan reviewed Feb 20, 2024

View reviewed changes

mariocynicys added 3 commits February 20, 2024 15:32

Revert "Refactor with_key_pair.rs to not use indices and use `&Unsign…

0df81c4

…edTransactionInput` instead" Revert the above commit since it introduces panics (`Vec[index]` this indexing technique might panic!). This reverts commit 926bc46.

onur's suggestions 1

f7bf4bc

mariocynicys removed the in progress label Feb 20, 2024

mariocynicys added 2 commits April 9, 2024 14:21

add test for filtering p2pk cached recently spent outputs

7152910

suggestions from dimxy: remove witness field in UnsignedTransactionIn…

5bee32a

…put is not used

mariocynicys requested a review from dimxy April 11, 2024 13:00

merge with origin/dev

eceaa5d

shamardy reviewed Apr 22, 2024

View reviewed changes

shamardy mentioned this pull request Apr 23, 2024

feat(hd_wallet): utxo and evm hd wallet and trezor #1962

Merged

shamardy self-requested a review May 6, 2024 14:56

mariocynicys added 6 commits May 7, 2024 12:28

merge with origin/dev

2ec84c4

patch/set prev_script as well in build_uncheck

a76d587

fix sign raw p2wpkh - different error is now emitted

a86b8a9

merge with origin/dev

f019f82

check for fee completeness before adding any input

c7a2cee

suggestions from omar: don't write a lot of json, use test helpers

11406e1

shamardy previously approved these changes May 8, 2024

View reviewed changes

mariocynicys dismissed shamardy’s stale review via 9cc7d16 May 8, 2024 11:08

mariocynicys force-pushed the p2pk-balances branch from 9cc7d16 to 2ecd5e4 Compare May 8, 2024 11:15

fix missing import in tests

fb1df6a

mariocynicys force-pushed the p2pk-balances branch from 2ecd5e4 to fb1df6a Compare May 8, 2024 11:22

mariocynicys added 2 commits May 10, 2024 21:49

refactor mm2_tests_inner

644b2ec

merge with origin/dev + fix failing tests

8679526

by adding -Segwit to coin name

dimxy approved these changes May 15, 2024

View reviewed changes

shamardy merged commit df6ab98 into GLEECBTC:dev May 15, 2024

cipig mentioned this pull request May 27, 2024

Add support for Verus single-currency smarttx spends #2127

Closed

shamardy mentioned this pull request Jul 19, 2024

HD Wallet implementation #1838

Open

This was referenced Jul 29, 2024

Request balance and utxos for both P2KH and P2PKH script hashes from Electrum. #720

Closed

chore(release): v2.1.0-beta #2043

Merged

		let output_script = try_f!(output_script(address));
		let mut output_scripts = vec![output_script];

	let output_script = try_f!(output_script(address));
	let mut output_scripts = vec![output_script];
	let mut output_scripts = vec![try_f!(output_script(address))];

	// DISCUSS: Should we really reduce this to pkh? what is it used for?
	// TODO: Discussion needed: "Should we really reduce this to pkh? what is it used for?"

		// Do we want to mix P2PK and non-P2PK spends?
		// Should we make another sweep P2PK method that spends all P2PK balance?

Conversation

mariocynicys commented Jan 18, 2024

Uh oh!

Uh oh!

mariocynicys commented Jan 23, 2024

Uh oh!

artemii235 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shamardy commented Feb 2, 2024

Uh oh!

shamardy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shamardy commented Feb 2, 2024

Uh oh!

mariocynicys commented Feb 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimxy Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mariocynicys Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shamardy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shamardy commented May 6, 2024

Uh oh!

mariocynicys commented May 8, 2024

Uh oh!

shamardy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shamardy commented May 8, 2024

Uh oh!

dimxy left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mariocynicys commented Feb 12, 2024 •

edited

Loading

dimxy Feb 26, 2024 •

edited

Loading

mariocynicys Feb 26, 2024 •

edited

Loading