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

[r2r] Refactor HW API #1405

Merged
merged 39 commits into from
Sep 13, 2022
Merged

[r2r] Refactor HW API #1405

merged 39 commits into from
Sep 13, 2022

Conversation

sergeyboyko0791
Copy link

@sergeyboyko0791 sergeyboyko0791 commented Jul 27, 2022

#964

  • Add EnumFromTrait and EnumFromInner derive macros
  • Rename WaitForTrezorPin user action to EnterTrezorPin
  • Rename ReadPublicKeyFromTrezor in progress status to WaitingForUserToConfirmPubkey
  • Replace HW related errors with crypto::HwRpcError for init_trezor, init_utxo, init_qtum, init_withdraw, init_scan_for_new_addresses RPCs

init_trezor

  • Add an optional expected device_pubkey to the init_trezor request
  • Add connected device_pubkey to the init_trezor_status result
  • init_trezor has to be able to reinitialize a Hardware wallet

init_utxo, init_qtum

  • Add ticker field to the init_utxo, init_qtum results
  • Remove unused error types

* Add an optional expected `device_pubkey` to the `init_trezor` request
* Add connected `device_pubkey` to the `init_trezor_status` result
* `init_trezor` has to be able to reinitialize a Hardware wallet
* Rename `WaitForTrezorPin` user action to `EnterTrezorPin`
* Rename `ReadPublicKeyFromTrezor` in progress status to `WaitingForUserToConfirmPubkey`
* Add `ticker` field to the `init_utxo`, `init_qtum` results
* Remove unused error types
* Add `crypto::HwRpcError`
* Replace HW related errors with `crypto::HwRpcError` for `init_utxo` RPC
* Replace HW related errors with `crypto::HwRpcError` for `init_withdraw` RPC
* Replace HW related errors with `crypto::HwRpcError` within `HDWalletRpcError` and `Mm2InitError`
* Add `EnumFromTrait` and `EnumFromInner` derive macros
* Refactor `get_new_address` to return an error if the last address is not used yet
* Split `HDWalletRpcError` into `GetNewAddressRpcError` and `CreateNewAccountRpcError`
* Add `task` RPC namespace, move all `init_*` methods there
* Add `EnterPassphrase` awaiting status
* Add `TrezorPassphrase` user action
* Rename `InProgressStatus::WaitingForUserToConfirmPubkey` to `InProgressStatus::FollowHwDeviceInstructions`
* Replace `Ready` status with `Ok` and `Error`
* Replace magic pubkey prefixes with `xpub` prefix on `TrezorSession::get_public_key`
* Create at least `min_addresses_number` addresses on coin activation
* Allow to generate up to `gap_limit` empty addresses on `get_new_address` RPC
* Improve `test_scan_for_new_addresses` test
* Add `test_can_get_new_address` test
* Implement `Debug` trait for `MmError` manually
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.

Thanks for the PR! I have one minor question for now.

mm2src/coins/rpc_command/get_new_address.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@shamardy shamardy 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! First review iteration, only some minor issues.

mm2src/coins/coin_balance.rs Outdated Show resolved Hide resolved
mm2src/coins/coin_balance.rs Outdated Show resolved Hide resolved
mm2src/coins/rpc_command/get_new_address.rs Outdated Show resolved Hide resolved
mm2src/coins/rpc_command/get_new_address.rs Outdated Show resolved Hide resolved
mm2src/coins/utxo.rs Outdated Show resolved Hide resolved
mm2src/trezor/src/utxo/utxo_command.rs Outdated Show resolved Hide resolved
mm2src/rpc_task/src/rpc_common.rs Outdated Show resolved Hide resolved
@sergeyboyko0791 sergeyboyko0791 changed the title [r2r] Refactor HW API [wip] Refactor HW API Sep 6, 2022
* Refactor `gen_new_addresses_if_required`
* Fix deserializing `min_addresses_number` from legacy UTXO request
* Remove `KEEP_XPUB_MAGIC` constant
@sergeyboyko0791
Copy link
Author

@artemii235 @shamardy this PR is ready for the next review.

@sergeyboyko0791 sergeyboyko0791 changed the title [wip] Refactor HW API [r2r] Refactor HW API Sep 9, 2022
@artemii235
Copy link
Member

artemii235 commented Sep 12, 2022

@sergeyboyko0791 It can't be [r2r] because tests do not even compile. Please check your PR's CI status before putting [r2r] mark.

UPD There are also conflicts that appeared after merging other PRs.

@sergeyboyko0791 sergeyboyko0791 changed the title [r2r] Refactor HW API [wip] Refactor HW API Sep 12, 2022
@sergeyboyko0791 sergeyboyko0791 changed the title [wip] Refactor HW API [r2r] Refactor HW API Sep 12, 2022
@sergeyboyko0791
Copy link
Author

@artemii235 now it's ready for the next review

Copy link
Collaborator

@shamardy shamardy 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
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.

🔥

@artemii235 artemii235 merged commit 50769ad into dev Sep 13, 2022
@artemii235 artemii235 deleted the refactor-hw-api branch September 13, 2022 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants