Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(hd_wallet): evm hd wallet and trezor #1979

Merged
merged 140 commits into from
Apr 22, 2024
Merged

Conversation

shamardy
Copy link
Collaborator

@shamardy shamardy commented Sep 29, 2023

Part of #1838

This is an in progress PR:

  • Refactor HD wallet functionalities to be separate from utxo so it can be used easily for other coins.
  • Add HDWallet derivation to EthCoin.
  • Implement HDWalletCoinOps, HDWalletBalanceOps, etc.. for EthCoin.
  • Balance streaming for all addresses feat(ETH): balance event streaming for ETH #2041 (comment)
  • Implement init_platform_coin_with_tokens using task manager for EthCoin.
  • Implement init_token using task manager.
  • Maintain backward compatibility.
  • Doc comments

Changes:

  • EVM balance streaming now contains address field to support HD wallet
  • chain_id is needed for all evm coins/tokens. chain_id should be part of platform coin config only actually and retrieved from platform coin, this is for v2 methods, but we should keep it in all coins config for v1 methods.

@shamardy shamardy added the in progress Changes will be made from the author label Sep 29, 2023
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!
It seems that few traits can be removed, which will hopefully make code simpler 🙂

mm2src/coins/hd_wallet/mod.rs Outdated Show resolved Hide resolved
mm2src/coins/hd_wallet/mod.rs Show resolved Hide resolved
mm2src/coins/hd_wallet/mod.rs Show resolved Hide resolved
mm2src/coins/hd_wallet/mod.rs Show resolved Hide resolved
mm2src/coins/hd_wallet/errors.rs Outdated Show resolved Hide resolved
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few minor notes 🙂

mm2src/coins/coin_balance.rs Outdated Show resolved Hide resolved
mm2src/coins/hd_wallet/mod.rs Outdated Show resolved Hide resolved
mm2src/coins/hd_wallet/mod.rs Outdated Show resolved Hide resolved
mm2src/coins/hd_wallet/mod.rs Outdated Show resolved Hide resolved
mm2src/coins/hd_wallet/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@shamardy
Copy link
Collaborator Author

shamardy commented Oct 9, 2023

Thanks you for the review @artemii235, I will continue working through this list #1979 (comment) in this sprint by adding new commits to this PR, so next review in a few weeks should be for commits after this comment only to make it easier to review :)

mm2src/coins/eth/eth_balance_events.rs Show resolved Hide resolved
mm2src/coins/eth/eth_hd_wallet.rs Outdated Show resolved Hide resolved
.collect()
}

// Todo: combine both implementations once worker threads are supported in WASM
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remind me please, as far as I remember we added worker support in wasm, right?
Do we need something else for this todo?

Copy link
Collaborator Author

@shamardy shamardy Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Workers need to be supported from the GUI side as well by adding a worker file for this and implementing any needed code in the worker file if there is any.

…vationMethod` and implement a default implementation for it

* Remove `my_addresses` from `utxo_tx_history_v2_common.rs` and use `all_addresses` in place of it
@shamardy
Copy link
Collaborator Author

shamardy commented Apr 8, 2024

@dimxy @laruh are you working on any more review iterations for this PR? If not can you please check if all comments are fixed and approve the PR so I can merge it.

@shamardy
Copy link
Collaborator Author

shamardy commented Apr 8, 2024

@onur-ozkan just need you to check this change #1979 (comment) and if it's alright by you.

Copy link
Member

@laruh laruh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🔥

Copy link
Collaborator

@dimxy dimxy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

* Add address_formatter higher-order function to HDWalletCoinOps to display eth addresses correctly
* add enable_eth_with_tokens_v2 test helper
* fix test_enable_eth_erc20_coins_with_enable_hd
* fix test_withdraw_and_send_hd_eth_erc20
* remove `wasm_test_withdraw_eth` and `test_send` since they use `ETH_DEV_NODE`
# ignore `swap_usdc_ibc_with_nimda` since it needs periodical funding
@shamardy shamardy merged commit 186bc95 into hd-account-balance Apr 22, 2024
26 of 27 checks passed
@shamardy shamardy deleted the evm-hd-wallet branch April 22, 2024 23:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants