Skip to content
This repository has been archived by the owner on Apr 15, 2024. It is now read-only.

Document Trezor Methods #403

Merged
merged 35 commits into from
Feb 8, 2023
Merged

Document Trezor Methods #403

merged 35 commits into from
Feb 8, 2023

Conversation

smk762
Copy link
Contributor

@smk762 smk762 commented Dec 3, 2022

closes: #392

  • task::init_trezor::init
  • task::init_trezor::status
  • task::init_trezor::user_action
  • task::enable_utxo::init
  • task::enable_utxo::status
  • task::enable_utxo::user_action
  • task::enable_qtum::init
  • task::enable_qtum::status
  • task::enable_qtum::user_action
    task::withdraw::init moved to other doc
    task::withdraw::status moved to other doc
    task::withdraw::cancel moved to other doc
  • task::account_balance::init
  • task::account_balance::status
  • task::account_balance::cancel
  • can_get_new_address
  • get_new_address

@smk762 smk762 self-assigned this Dec 3, 2022
@smk762 smk762 marked this pull request as draft December 3, 2022 04:34
@smk762 smk762 marked this pull request as ready for review December 20, 2022 15:47
Copy link
Contributor

@sergeyboyko0791 sergeyboyko0791 left a comment

Choose a reason for hiding this comment

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

A huge work! Thank you!
Just a few notes and suggestions

docs/basic-docs/atomicdex-api-20-dev/trezor_integration.md Outdated Show resolved Hide resolved
docs/basic-docs/atomicdex-api-20-dev/trezor_integration.md Outdated Show resolved Hide resolved
docs/basic-docs/atomicdex-api-20-dev/trezor_integration.md Outdated Show resolved Hide resolved
Comment on lines 828 to 830
#### Arguments

| Parameter | Type | Description |
Copy link
Contributor

Choose a reason for hiding this comment

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

The request payload is the same for task::enable_qtum::init and task::enable_utxo::init. I'm not sure if this is a good idea, but what do you think about declaring a TaskEnableUtxoInit (not the best name) payload and just link it within task\_enable\_qtum\_init and task\_enable\_utxo\_init methods?
I don't think that the payload will be different in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think in general where request payloads are the same (or 80% same with the remaining optional) it is good to have a single method that can be used for both. It reduces confusion from user and overhead to create maintain docs.

IMO the ideal solution would be a single "enable_coin" method which can be used on anything, with API + coins file able to determine which underlying method to use. Another enhancement would be allowing user to define coins_config.json and derive the rpc_nodes / electrums etc from there - though I understand this would be a fairly big job and maybe better to plan towards for v3 of the API

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't answer this suggestion correctly as I didn't work on enable V2 feature, so I'll just reference to this comment KomodoPlatform/komodo-defi-framework#1006 (comment) and to your comment KomodoPlatform/komodo-defi-framework#1006 (comment) 🙂

Comment on lines 946 to 948
#### Response (ready, successful, Trezor mode)

| Parameter | Type | Description |
Copy link
Contributor

Choose a reason for hiding this comment

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

Response payload could be also declared somewhere above

docs/basic-docs/atomicdex-api-20-dev/trezor_integration.md Outdated Show resolved Hide resolved
docs/basic-docs/atomicdex-api-20-dev/trezor_integration.md Outdated Show resolved Hide resolved
docs/basic-docs/atomicdex-api-20-dev/trezor_integration.md Outdated Show resolved Hide resolved
@smk762
Copy link
Contributor Author

smk762 commented Jan 4, 2023

I've made some updates to resolve review feedback - just waiting on @Canialon to provide an error response example for FoundMultipleDevices and it will be ready for re-review

@Canialon
Copy link
Contributor

Canialon commented Jan 4, 2023

I've made some updates to resolve review feedback - just waiting on @Canialon to provide an error response example for FoundMultipleDevices and it will be ready for re-review

{
  "mmrpc": "2.0",
  "result": {
    "status": "Error",
    "details": {
      "error": "Found multiple devices. Please unplug unused devices",
      "error_path": "init_hw.crypto_ctx.hw_client",
      "error_trace": "init_hw:151] crypto_ctx:248] crypto_ctx:354] hw_client:152] hw_client:126]",
      "error_type": "HwError",
      "error_data": "FoundMultipleDevices"
    }
  },
  "id": null
}

@Canialon
Copy link
Contributor

Canialon commented Jan 5, 2023

Looks like it is no trezor_integration page there.. it wasn’t added to the sidebar
image

@smk762
Copy link
Contributor Author

smk762 commented Jan 5, 2023

Reference

Thanks, sidebar entries should be visible now.

@smk762
Copy link
Contributor Author

smk762 commented Jan 5, 2023

fixed a little syntax that was causing error, should be good now 🤞

@Canialon
Copy link
Contributor

Canialon commented Jan 5, 2023

fixed a little syntax that was causing error, should be good now 🤞

The sidebar works good now! thanks!!

@smk762
Copy link
Contributor Author

smk762 commented Jan 7, 2023

There appears to be duplication of the withdraw task methods between this PR and #395, which is ready for re-review. Once we merge #395, I'll merge master into this branch and tweak the structure to use links instead of duplicated sections. Setting this to draft until then.

@smk762 smk762 marked this pull request as ready for review January 11, 2023 08:51
@smk762
Copy link
Contributor Author

smk762 commented Jan 11, 2023

I've broken up this doc into sections, as some methods are not Trezor specific (e.g. coin activation and withdrawal tasks are possible without a Trezor). Sidebar now looks like below
image

@Canialon
Copy link
Contributor

I believe it makes sense to add a coin_activation_tasks link to the address_managment and account balance_tasks pages

@smk762 smk762 merged commit e709ecb into master Feb 8, 2023
@smk762 smk762 deleted the trezor_integration branch February 8, 2023 12:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Hardware wallet docs
4 participants