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

dimxy · 2023-09-06T14:52:17Z

Closes #1270
As suggested in the issue, script_type field is added to Address structure so we do not need the extra script type param in output_script() any more. Now the script type is determined right at parsing input address strings. For that we always need coin prefixes when we parse addresses as strings, so the standard Address::from_str() was replaced on Address::from_legacyaddress() function, with coin prefixes as params.
The output_script() fn is now universally used everywhere when output script should be created from an address

There are still few cases when we need the standard from_str fn (w/o prefixes) to parse addresses as strings, f.e. the convertaddress rpc. For this a new LegacyAddress type is added which supports the from_str() method.

artemii235

Thanks for the PR! I have few changes requests, all related to signing.

mm2src/coins/lightning/ln_events.rs

mm2src/coins/qrc20.rs

mm2src/coins/utxo.rs

mm2src/coins/utxo/qtum_delegation.rs

mm2src/coins/utxo/slp.rs

mm2src/coins/utxo/utxo_tests.rs

dimxy · 2023-09-08T17:04:25Z

Added missed fix for segwit signing code (thanks @artemii235 for noticing that).
The explanation is: as now a unified output_script fn is used for all cases when prev_script is created, for segwit we pass scriptPubKey (not scriptCode as before) to the signing code. The segwit signing code creates scriptCode itself. I believe this is more similar to how the original signing code in daemons does this.

(Seems tests are better now. There are still few tests failing but I believe because of this PR)

shamardy · 2023-09-08T17:13:33Z

(Seems tests are better now. There are still few tests failing but I believe because of this PR)

Yeah, failing tests now are not due to changes in this PR.

artemii235

Few more suggestions, please consider them as non-blocking.

mm2src/mm2_bitcoin/rpc/src/v1/types/mod.rs

artemii235 · 2023-09-11T07:28:11Z

mm2src/coins/qrc20.rs

+        coin.as_ref().conf.pub_addr_prefix,
+        coin.as_ref().conf.pub_t_addr_prefix,
+        coin.as_ref().conf.p2sh_addr_prefix,
+        coin.as_ref().conf.p2sh_t_addr_prefix,


What do you think about creating the following struct and using it instead to make code a bit shorted/avoid duplication?

struct AddressPrefixes { pub_addr_prefix: u8, pub_t_addr_prefix: u8, p2sh_addr_prefix: u8, p2sh_t_addr_prefix: u8, }

So this and other from_legacyaddress calls will look like

UtxoAddress::from_legacyaddress(&req.to, coin.as_ref().conf.address_prefixes)

yes, I also thought about a struct like that or array. I will do this, thank you

What would you say about using Vec for prefixes, like it is in daemons codebases. So such a vec would look like [t_addr_prefix, p2pkh_prefix] with t_addr_prefix optional.

pub struct Address { /// The prefixes of the p2pkh address of size 1 or 2 pub p2pkh_prefix: Vec<u8>, /// The prefixes of the p2sh address of size 1 or 2 pub p2sh_prefix: Vec<u8>, ... }

Functions would accept slices in their params.
I think it would be easier to deal with such types (less variables or members)
Also we do not have t_addr_prefix in most cases, so we do not process it with this approach.
Plus this is consistent with other prefixes representations like xpub

Added NetworkAddressPrefixes and AddressPrefixes structs to encapsulate prefixes

artemii235 · 2023-09-11T07:37:41Z

mm2src/coins/qrc20/qrc20_tests.rs

@@ -150,7 +152,7 @@ fn test_validate_maker_payment() {

    assert_eq!(
        *coin.utxo.derivation_method.unwrap_single_addr(),
-        "qUX9FGHubczidVjWPCUWuwCUJWpkAtGCgf".into()
+        Address::from_legacyaddress("qUX9FGHubczidVjWPCUWuwCUJWpkAtGCgf", 120, 0, 50, 0).unwrap()


I think you can use coin.as_ref().conf.pub_addr_prefix/... here and other similar lines. Also, QTUM regtest P2SH prefix is 110.

I tried as_ref but I guess deref here is okay, the alternative is to use '&Address::from_legacyaddress(..).
Also instead of using consts I changed it to coin.as_ref().conf.address_prefixes

artemii235 · 2023-09-11T07:41:30Z

mm2src/mm2_bitcoin/keys/src/legacyaddress.rs

+impl From<&'static str> for LegacyAddress {
+    fn from(s: &'static str) -> Self { s.parse().unwrap() } // TODO: dangerous unwrap?
+}


You can likely add #cfg[test] to make it test-only and avoid occasional usage and panic in production code.

I could not use cfg[test] as this code used in tests in other modules but I added Default instead

artemii235 · 2023-09-11T07:45:42Z

mm2src/coins/utxo/utxo_common_tests.rs

+        Address::from_legacyaddress("RG278CfeNPFtNztFZQir8cgdWexVhViYVy", 60, 0, 85, 0).unwrap(),
+        Address::from_legacyaddress("RYPz6Lr4muj4gcFzpMdv3ks1NCGn3mkDPN", 60, 0, 85, 0).unwrap(),
+        Address::from_legacyaddress("RJeDDtDRtKUoL8BCKdH7TNCHqUKr7kQRsi", 60, 0, 85, 0).unwrap(),
+        Address::from_legacyaddress("RQHn9VPHBqNjYwyKfJbZCiaxVrWPKGQjeF", 60, 0, 85, 0).unwrap(),


It is preferred to use constants like const KMD_P2PKH: u8 = 60; and so on.

added contants (see address_prefixes.rs)

dimxy · 2023-10-09T17:42:45Z

I hope, I have fixed the requests above

laruh · 2023-10-31T04:50:30Z

@dimxy Hi, this PR started to have conflicts.

…s deserializaton)

fix output_script fn to use script_type add LegacyAddress obj allowing from_str deser use parametrised Address::from_legacyaddress instead of default from_str use LegacyAddress in convertaddress fix some clippy errors

…ddress (different error processing)

…tness hash

dimxy · 2024-01-29T15:20:21Z

fixed review notes, added AddressBuilder and divided the witness script creation fn into two functions separately for wpkh and wsh (for better address hash checking)

shamardy

Thanks you for the fixes! Next review iteration!

mm2src/coins/lightning/ln_events.rs

mm2src/coins/qrc20.rs

mm2src/mm2_bitcoin/keys/src/address/address_builder.rs

shamardy · 2024-02-01T09:57:04Z

@mariocynicys you can give this PR a look/review since changes here will affect your PR #2053 a lot and this is to be merged first.

shamardy

Next review iteration! Probably the last review iteration unless there are some minor things found during the last code check :)

mm2src/mm2_bitcoin/keys/src/address/address_builder.rs

mm2src/mm2_bitcoin/script/src/builder.rs

mm2src/coins/utxo.rs

mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs

mm2src/coins/utxo/utxo_withdraw.rs

mariocynicys

Thanks!
I only have a couple of nits here.

mm2src/mm2_bitcoin/keys/src/address_prefixes.rs

mm2src/mm2_bitcoin/keys/src/address.rs

mm2src/mm2_bitcoin/keys/src/address_prefixes.rs

mm2src/mm2_bitcoin/keys/src/address.rs

shamardy

🔥

mariocynicys

Thanks, LGTM!

shamardy · 2024-02-06T17:47:22Z

This is just a refactor PR and it's needed for other PRs, so sec-review can be done in release PR.

* evm-hd-wallet: (27 commits) Fix todo comments Fix HDAddressOps::Address trait bounds fix(indexeddb): fix IDB cursor.continue_() call after drop (KomodoPlatform#2028) security bump for `h2` (KomodoPlatform#2062) fix(makerbot): allow more than one prices url in makerbot (KomodoPlatform#2027) fix(wasm worker env): refactor direct usage of `window` (KomodoPlatform#1953) feat(nft): nft abi in withdraw_nft RPC, clear_nft_db RPC (KomodoPlatform#2039) refactor(utxo): refactor utxo output script creation (KomodoPlatform#1960) feat(ETH): balance event streaming for ETH (KomodoPlatform#2041) chore(release): bump mm2 version to 2.1.0-beta (KomodoPlatform#2044) feat(trezor): add segwit support for withdraw with trezor (KomodoPlatform#1984) chore(config): remove vscode launchjson (KomodoPlatform#2040) feat(trading-proto-upgrade): wasm DB, kickstart, refund states, v2 RPCs (KomodoPlatform#2015) feat(UTXO): balance event streaming for Electrum clients (KomodoPlatform#2013) feat(tx): add new sign_raw_transaction rpc for UTXO and EVM coins (KomodoPlatform#1930) fix(p2p): handle encode_and_sign errors (KomodoPlatform#2038) chore(release): add changelog entries for v2.0.0-beta (KomodoPlatform#2037) chore(network): write network information to stdout (KomodoPlatform#2034) fix(price_endpoints): add cached url (KomodoPlatform#2032) deps(network): sync with upstream yamux (KomodoPlatform#2030) ...

* dev: feat(zcoin): ARRR WASM implementation (KomodoPlatform#1957) feat(trading-proto-upgrade): locked amounts, kmd burn and other impl (KomodoPlatform#2046) fix(indexeddb): set stop on success cursor condition (KomodoPlatform#2067) feat(config): add `max_concurrent_connections` to mm2 config (KomodoPlatform#2063) feat(stats_swaps): add gui/mm_version in stats db (KomodoPlatform#2061) fix(indexeddb): fix IDB cursor.continue_() call after drop (KomodoPlatform#2028) security bump for `h2` (KomodoPlatform#2062) fix(makerbot): allow more than one prices url in makerbot (KomodoPlatform#2027) fix(wasm worker env): refactor direct usage of `window` (KomodoPlatform#1953) feat(nft): nft abi in withdraw_nft RPC, clear_nft_db RPC (KomodoPlatform#2039) refactor(utxo): refactor utxo output script creation (KomodoPlatform#1960) feat(ETH): balance event streaming for ETH (KomodoPlatform#2041) chore(release): bump mm2 version to 2.1.0-beta (KomodoPlatform#2044) feat(trezor): add segwit support for withdraw with trezor (KomodoPlatform#1984)

dimxy changed the title ~~feat(utxo) Refactor utxo output script creation~~ feat(utxo): Refactor utxo output script creation Sep 6, 2023

dimxy changed the title ~~feat(utxo): Refactor utxo output script creation~~ refactor(utxo): Refactor utxo output script creation Sep 6, 2023

dimxy changed the title ~~refactor(utxo): Refactor utxo output script creation~~ refactor(utxo): refactor utxo output script creation Sep 6, 2023

dimxy added the in progress Changes will be made from the author label Sep 6, 2023

artemii235 self-requested a review September 7, 2023 08:00

dimxy requested a review from shamardy September 8, 2023 10:15

dimxy added under review and removed in progress Changes will be made from the author labels Sep 8, 2023

artemii235 requested changes Sep 8, 2023

View reviewed changes

artemii235 reviewed Sep 11, 2023

View reviewed changes

dimxy mentioned this pull request Sep 15, 2023

feat(tx): add new sign_raw_transaction rpc for UTXO and EVM coins #1930

Merged

dimxy force-pushed the feature-add-script-type-to-address branch 2 times, most recently from e1e72df to 20570bc Compare October 9, 2023 17:12

dimxy added 13 commits November 2, 2023 16:10

remove unused TransactionOutputs (which prevented parametrised addres…

d5f055a

…s deserializaton)

add script_type to Address

19cab7a

fix output_script fn to use script_type add LegacyAddress obj allowing from_str deser use parametrised Address::from_legacyaddress instead of default from_str use LegacyAddress in convertaddress fix some clippy errors

fix format

ca5e9c2

fix coin prefix in address tests

753fb93

fix fmt

9cc277f

fix test results because of Address::from_str changed to from_legacya…

1793290

…ddress (different error processing)

add NetworkAddressPrefixes type and replace use of u8 addr prefixes

f953499

remove controversial check in withdraw (no segwit case)

6050621

fix use for coin prefixes in test helpers

0b1842d

add default val for LegacyAddress

82a3b2d

remove commented code

6924cb0

remove unused invalid prefixes error

3de99ae

fix after rebase

838911d

dimxy added 5 commits January 29, 2024 13:44

fix addr_format for check_if_my_payment_sent fn (must not be segwit)

a38ed54

rename address builder options to avoid confusion

35367d4

break script witness creation fn into p2wpkh and p2wsh to validate wi…

f66529e

…tness hash

add swap events debug logging

9206ca2

fix clippy fmt

9bb5f13

dimxy added 2 commits January 29, 2024 23:13

remove my debug in swaps

acf7174

remove temp ignore for iris tests

095a804

shamardy reviewed Jan 31, 2024

View reviewed changes

mm2src/coins/lightning/ln_events.rs Outdated Show resolved Hide resolved

mm2src/coins/qrc20.rs Show resolved Hide resolved

mm2src/mm2_bitcoin/keys/src/address/address_builder.rs Outdated Show resolved Hide resolved

shamardy requested a review from mariocynicys February 1, 2024 09:56

refactor AddressBuilder according to builder pattern

64dbf87

shamardy reviewed Feb 2, 2024

View reviewed changes

shamardy mentioned this pull request Feb 2, 2024

fix(p2pk): show P2PK balance #2053

Merged

dimxy added 3 commits February 2, 2024 18:23

fixed review notes: propagate output_script() error

354c970

remove extra map_err, add returned err logging

4603855

remove extra map_to_mm

9260531

mariocynicys reviewed Feb 5, 2024

View reviewed changes

dimxy added 2 commits February 5, 2024 14:14

refactor AddressPrefixes - remove extra const type

3434e2b

refactor: rename AddressPrefixes to singular

d7f6cdb

shamardy approved these changes Feb 5, 2024

View reviewed changes

mariocynicys approved these changes Feb 6, 2024

View reviewed changes

shamardy added 2.1.0-beta sec-review no-deps labels Feb 6, 2024

shamardy merged commit ed80898 into dev Feb 6, 2024
26 of 33 checks passed

shamardy deleted the feature-add-script-type-to-address branch February 6, 2024 17:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dimxy commented Sep 6, 2023

artemii235 left a comment

dimxy commented Sep 8, 2023

shamardy commented Sep 8, 2023

artemii235 left a comment

artemii235 Sep 11, 2023

dimxy Sep 11, 2023

dimxy Oct 5, 2023

dimxy Oct 9, 2023

artemii235 Sep 11, 2023

dimxy Oct 9, 2023

artemii235 Sep 11, 2023

dimxy Oct 9, 2023

artemii235 Sep 11, 2023

dimxy Oct 9, 2023 •

edited

Loading

dimxy commented Oct 9, 2023

laruh commented Oct 31, 2023

dimxy commented Jan 29, 2024

shamardy left a comment

shamardy commented Feb 1, 2024

shamardy left a comment

mariocynicys left a comment

shamardy left a comment

mariocynicys left a comment

shamardy commented Feb 6, 2024

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

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

Conversation

dimxy commented Sep 6, 2023

artemii235 left a comment

Choose a reason for hiding this comment

dimxy commented Sep 8, 2023

shamardy commented Sep 8, 2023

artemii235 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimxy Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

dimxy commented Oct 9, 2023

laruh commented Oct 31, 2023

dimxy commented Jan 29, 2024

shamardy left a comment

Choose a reason for hiding this comment

shamardy commented Feb 1, 2024

shamardy left a comment

Choose a reason for hiding this comment

mariocynicys left a comment

Choose a reason for hiding this comment

shamardy left a comment

Choose a reason for hiding this comment

mariocynicys left a comment

Choose a reason for hiding this comment

shamardy commented Feb 6, 2024

dimxy Oct 9, 2023 •

edited

Loading