fix(hd_wallet): make extended pubkey of hd wallet generic #2159

shamardy · 2024-07-08T22:15:14Z

No description provided.

mariocynicys

Have a discussion point down there.
This LGTM though :)

mariocynicys · 2024-07-09T07:21:36Z

mm2src/coins/eth/eth_hd_wallet.rs

        &self,
-        extended_pubkey: &Secp256k1ExtendedPublicKey,
+        extended_pubkey: &HDCoinExtendedPubkey<Self>,
        derivation_path: DerivationPath,


A general Q regarding the typing choices in the HD code (not a blocker for this PR):
Is it really better to have the types with maps from Self like this. I mean we are implementing the trait for a concrete type now so we could use the concrete arguments for more clarity.

Also could the names for such maps be followed by Of (e.g. HDCoinExtendedPubkeyOf<Coin>). The confusion for me personally comes from that I always think about these types with generic args that they contain these args but here they end up digging up the args and extract some other type from them.

let me label these as nits as they might be too opinionated.

I mean we are implementing the trait for a concrete type now so we could use the concrete arguments for more clarity.

Did that for some of the implementations, hope it's a bit clearer now. If you find any other easy refactors please point me to it, I wanted to make all the below types private to the hd_wallet mod but it requires a lot of refactors. Should be done in the future in a separate PR.

komodo-defi-framework/mm2src/coins/hd_wallet/mod.rs

Lines 52 to 64 in 76e87ab

pub(crate) type HDAccountsMap<HDAccount> = BTreeMap<u32, HDAccount>;

pub(crate) type HDAccountsMutex<HDAccount> = AsyncMutex<HDAccountsMap<HDAccount>>;

pub(crate) type HDAccountsMut<'a, HDAccount> = AsyncMutexGuard<'a, HDAccountsMap<HDAccount>>;

pub(crate) type HDAccountMut<'a, HDAccount> = AsyncMappedMutexGuard<'a, HDAccountsMap<HDAccount>, HDAccount>;

type HDWalletHDAddress<T> = <<T as HDWalletOps>::HDAccount as HDAccountOps>::HDAddress;

type HDCoinHDAddress<T> = HDWalletHDAddress<<T as HDWalletCoinOps>::HDWallet>;

pub(crate) type HDWalletAddress<T> =

<<<T as HDWalletOps>::HDAccount as HDAccountOps>::HDAddress as HDAddressOps>::Address;

pub(crate) type HDCoinAddress<T> = HDWalletAddress<<T as HDWalletCoinOps>::HDWallet>;

type HDWalletExtendedPubkey<T> = <<T as HDWalletOps>::HDAccount as HDAccountOps>::ExtendedPublicKey;

pub(crate) type HDCoinExtendedPubkey<T> = HDWalletExtendedPubkey<<T as HDWalletCoinOps>::HDWallet>;

pub(crate) type HDCoinHDAccount<T> = HDWalletHDAccount<<T as HDWalletCoinOps>::HDWallet>;

type HDWalletHDAccount<T> = <T as HDWalletOps>::HDAccount;

dimxy · 2024-07-11T07:56:45Z

An idea:
I think, as this new ExtendedPublicKey type is used in many traits and functions, we could implement it not as an associated type but as a dedicated enum type (when it is an associated type we have to create bulky type definitions).

I made draft code with ExtendedPublicKey as enum (named UniExtendedPublicKey)

Otherwise LGTM

shamardy · 2024-07-12T10:49:53Z

An idea:
I think, as this new ExtendedPublicKey type is used in many traits and functions, we could implement it not as an associated type but as a dedicated enum type (when it is an associated type we have to create bulky type definitions).

This will make the code much simpler, but as discussed with you on DM, if we wanted to separate HD wallet code to a separate crate in the future to be used by anyone else, associated types will be much easier to extend without requiring PRs in komodefi.

…for coins

mariocynicys · 2024-07-12T13:04:48Z

@dimxy @shamardy gonna disagree with you here.
I think dyn Traits and associate types > enums. merely for the fact that one won't need to match the enum (even when we know what it matches to) before using it.

mariocynicys

One last concrete type, LGTM otherwise :)

mariocynicys · 2024-07-12T13:39:42Z

mm2src/coins/utxo/qtum.rs

@@ -1112,7 +1112,7 @@ impl HDWalletBalanceOps for QtumCoin {

    async fn all_known_addresses_balances(
        &self,
-        hd_account: &HDCoinHDAccount<Self>,
+        hd_account: &UtxoHDAccount,
    ) -> BalanceResult<Vec<HDAddressBalance<Self::BalanceObject>>> {
        utxo_common::all_known_addresses_balances(self, hd_account).await
    }


two lines under, known_address_balance still uses HDBalanceAddress<Self>.

komodo-defi-framework/mm2src/coins/utxo/qtum.rs

Line 1120 in eb99106

async fn known_address_balance(&self, address: &HDBalanceAddress<Self>) -> BalanceResult<Self::BalanceObject> {

Thanks for noticing this, will push a fix asap.

mariocynicys

LGTM!

dimxy · 2024-07-12T19:10:17Z

@dimxy @shamardy gonna disagree with you here. I think dyn Traits and associate types > enums. merely for the fact that one won't need to match the enum (even when we know what it matches to) before using it.

A enum provides static polymorphism so could not be extended w/o its code modification, so we agreed an assoc type is better for hdwallet because it allows to add new implementations.

* dev: feat(nft-swap): add standalone maker contract and proxy support (#2100) feat(ETH): add `gas_limit` coins param to override default values (#2137) feat(tendermint): implement better sequence resolving logic (#2164) ci(artifact): add target for macos on apple silicon (#2163) fix(helpers): extend http to ws address conversion (#2166) fix(makerbot): add "testcoin" to provider options (#2161) fix(hd_wallet): make extended pubkey of hd wallet generic (#2159) fix(docker-tests): implement containers runtime directories (#2162) feat(tendermint): improve the `max` handling for tendermint withdraw (#2155) revert #2158 (comment) (#2160) ci(artifacts): upload build artifacts with in-tree script (#2158) test(tendermint): migrate to local/offline containerized testnets (#2128) use easingthemes/ssh-deploy@v5.0.3 for all builds except windows (#2157) chore(bin): rename mm2 binaries to kdf (#2126)

* dev: (22 commits) chore(release): bump mm2 version to 2.2.0-beta (KomodoPlatform#2188) ci(docker-tests): ignore tendermint IBC tests for now (KomodoPlatform#2185) feat(nft-swap): complete refund methods (KomodoPlatform#2129) chore(release): add changelog entries for v2.1.0-beta (KomodoPlatform#2165) fix(zcoin): don't force low r signing to generate htlc pubkey for zcoin (KomodoPlatform#2184) chore(rust-analyzer): add rust-analyzer into the workspace toolchain (KomodoPlatform#2179) chore: migrate .cargo/config to .cargo/config.toml to avoid deprecation warning (KomodoPlatform#2177) fix(swaps): ensure taker payment spend confirmations (KomodoPlatform#2176) feat(nft-swap): add standalone maker contract and proxy support (KomodoPlatform#2100) feat(ETH): add `gas_limit` coins param to override default values (KomodoPlatform#2137) feat(tendermint): implement better sequence resolving logic (KomodoPlatform#2164) ci(artifact): add target for macos on apple silicon (KomodoPlatform#2163) fix(helpers): extend http to ws address conversion (KomodoPlatform#2166) fix(makerbot): add "testcoin" to provider options (KomodoPlatform#2161) fix(hd_wallet): make extended pubkey of hd wallet generic (KomodoPlatform#2159) fix(docker-tests): implement containers runtime directories (KomodoPlatform#2162) feat(tendermint): improve the `max` handling for tendermint withdraw (KomodoPlatform#2155) revert KomodoPlatform#2158 (comment) (KomodoPlatform#2160) ci(artifacts): upload build artifacts with in-tree script (KomodoPlatform#2158) test(tendermint): migrate to local/offline containerized testnets (KomodoPlatform#2128) ...

shamardy added in progress and removed in progress labels Jul 8, 2024

shamardy requested review from dimxy and mariocynicys July 8, 2024 22:38

make extended pubkey of hd wallet generic

bde2077

onur-ozkan force-pushed the feat-generic-xpub branch from 607d9fe to bde2077 Compare July 9, 2024 06:36

mariocynicys previously approved these changes Jul 9, 2024

View reviewed changes

dimxy previously approved these changes Jul 11, 2024

View reviewed changes

shamardy added in progress and removed under review labels Jul 12, 2024

shamardy added 2 commits July 12, 2024 15:44

review fixes: use concrete types for HD wallet trait implementations …

76e87ab

…for coins

Merge remote-tracking branch 'origin/dev' into feat-generic-xpub

eb99106

shamardy dismissed stale reviews from dimxy and mariocynicys via eb99106 July 12, 2024 12:45

shamardy added under review and removed in progress labels Jul 12, 2024

shamardy requested a review from mariocynicys July 12, 2024 12:48

mariocynicys reviewed Jul 12, 2024

View reviewed changes

review fixes: use concrete type for qtum known_address_balance

6a30fcb

mariocynicys approved these changes Jul 12, 2024

View reviewed changes

shamardy merged commit 6d1342e into dev Jul 12, 2024
22 of 25 checks passed

shamardy deleted the feat-generic-xpub branch July 12, 2024 15:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(hd_wallet): make extended pubkey of hd wallet generic #2159

fix(hd_wallet): make extended pubkey of hd wallet generic #2159

shamardy commented Jul 8, 2024

mariocynicys left a comment

mariocynicys Jul 9, 2024

mariocynicys Jul 9, 2024

mariocynicys Jul 9, 2024

shamardy Jul 12, 2024 •

edited

Loading

dimxy commented Jul 11, 2024 •

edited

Loading

shamardy commented Jul 12, 2024

mariocynicys commented Jul 12, 2024

mariocynicys left a comment

mariocynicys Jul 12, 2024

shamardy Jul 12, 2024

mariocynicys left a comment

dimxy commented Jul 12, 2024

	pub(crate) type HDAccountsMap<HDAccount> = BTreeMap<u32, HDAccount>;
	pub(crate) type HDAccountsMutex<HDAccount> = AsyncMutex<HDAccountsMap<HDAccount>>;
	pub(crate) type HDAccountsMut<'a, HDAccount> = AsyncMutexGuard<'a, HDAccountsMap<HDAccount>>;
	pub(crate) type HDAccountMut<'a, HDAccount> = AsyncMappedMutexGuard<'a, HDAccountsMap<HDAccount>, HDAccount>;
	type HDWalletHDAddress<T> = <<T as HDWalletOps>::HDAccount as HDAccountOps>::HDAddress;
	type HDCoinHDAddress<T> = HDWalletHDAddress<<T as HDWalletCoinOps>::HDWallet>;
	pub(crate) type HDWalletAddress<T> =
	<<<T as HDWalletOps>::HDAccount as HDAccountOps>::HDAddress as HDAddressOps>::Address;
	pub(crate) type HDCoinAddress<T> = HDWalletAddress<<T as HDWalletCoinOps>::HDWallet>;
	type HDWalletExtendedPubkey<T> = <<T as HDWalletOps>::HDAccount as HDAccountOps>::ExtendedPublicKey;
	pub(crate) type HDCoinExtendedPubkey<T> = HDWalletExtendedPubkey<<T as HDWalletCoinOps>::HDWallet>;
	pub(crate) type HDCoinHDAccount<T> = HDWalletHDAccount<<T as HDWalletCoinOps>::HDWallet>;
	type HDWalletHDAccount<T> = <T as HDWalletOps>::HDAccount;

fix(hd_wallet): make extended pubkey of hd wallet generic #2159

fix(hd_wallet): make extended pubkey of hd wallet generic #2159

Conversation

shamardy commented Jul 8, 2024

mariocynicys left a comment

Choose a reason for hiding this comment

mariocynicys Jul 9, 2024

Choose a reason for hiding this comment

mariocynicys Jul 9, 2024

Choose a reason for hiding this comment

mariocynicys Jul 9, 2024

Choose a reason for hiding this comment

shamardy Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

dimxy commented Jul 11, 2024 • edited Loading

shamardy commented Jul 12, 2024

mariocynicys commented Jul 12, 2024

mariocynicys left a comment

Choose a reason for hiding this comment

mariocynicys Jul 12, 2024

Choose a reason for hiding this comment

shamardy Jul 12, 2024

Choose a reason for hiding this comment

mariocynicys left a comment

Choose a reason for hiding this comment

dimxy commented Jul 12, 2024

shamardy Jul 12, 2024 •

edited

Loading

dimxy commented Jul 11, 2024 •

edited

Loading