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

Utils, SettingsInfo, WizardCreateDevice1: use minimum value for restore height of Ledger and Trezor #3573

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

rating89us
Copy link
Contributor

@rating89us rating89us commented Jun 18, 2021

Use minimum restore height values for each device model, according to when they started supporting Monero (see monero-project/monero#6797).

This minimum value for restore height is only set if the user doesn't set a restore height.

Ledger Nano S support:

Trezor Model T support:

@rating89us rating89us force-pushed the patch-92 branch 2 times, most recently from 1573afa to b057428 Compare June 18, 2021 14:30
Copy link
Collaborator

@selsta selsta left a comment

Choose a reason for hiding this comment

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

  • As far as I can see this ignores Testnet / Stagnet, we would only want this on Mainnet
  • IMO if someone enters a specific block height we should also set this block height. Only if someone keeps the restore height empty then we should use this minimum value for Ledger / Trezor instead of 0.

@rating89us
Copy link
Contributor Author

we would only want this on Mainnet

Fixed

IMO if someone enters a specific block height we should also set this block height. Only if someone keeps the restore height empty then we should use this minimum value for Ledger / Trezor instead of 0.

I disagree. Why should we let users sync unnecessary blocks from 2014-04 to 2018-03/2018-11? Just for the sake of respecting the restore height entered by the user? Why would a user want to sync 4 years of unnecessary blocks?

@selsta
Copy link
Collaborator

selsta commented Jun 19, 2021

Just for the sake of respecting the restore height entered by the user?

I personally dislike software that overwrites my entry.

Why would a user want to sync 4 years of unnecessary blocks?

I can imagine that someone converts their existing paper wallet monero seed into a Ledger seed. In this case it is possible that they have to sync earlier than "2018-03-04". Also some used Ledger before it was publicly available, including me. I don't know if my first transaction was before 2018-03-04, but I definitely used it before release. This are just 2 quick idea where it could be necessary.

I will also ask @dEBRUYNE-1 as he had this idea initially.

@dEBRUYNE-1
Copy link
Contributor

The coded restore height should arguably only be used if the user enters 0 or 1 as value or if no value is set. There are a few scenarios where a user would desire to use a height that is in advance of the suggested height, hence my recommendation to only use the coded height in aforementioned scenario.

@rating89us
Copy link
Contributor Author

rating89us commented Jun 25, 2021

Made the following changes:

WizardCreateDevice:

  • Default value of Restore height field is now blank (it was "0")
  • If restore height field is blank & Trezor: use restore height 1697500
  • If restore height field is blank & Ledger: use restore height 1522000
  • Changed restore height field label to: "Wallet creation date or restore height (optional)"
  • Changed restore height field placeholder to: "Date (YYYY-MM-DD) or restore height. Leave blank if you don't know."

SettingsInfo

  • Restore height field now accepts blank value ("") as restore height

js/Utils.js Outdated Show resolved Hide resolved
js/Utils.js Outdated Show resolved Hide resolved
wizard/WizardCreateDevice1.qml Outdated Show resolved Hide resolved
@rating89us
Copy link
Contributor Author

There is still a bug to be fixed (I think it was already present before this PR): if you set restore height = 0 on WizardCreateDevice1.qml (I tested on Trezor only), the restore height is set to current restore height.

@selsta
Copy link
Collaborator

selsta commented Jun 25, 2021

I think that's intentional. Will have to look at the code again but we want to set it to 1 if someone enters 0.

@rating89us
Copy link
Contributor Author

but if you enter restore height = 0 on SettingsInfo, it respects the user input

@selsta
Copy link
Collaborator

selsta commented Jun 25, 2021

That's most likely a bug then.

@rating89us
Copy link
Contributor Author

I observed this behavior only when you restore a hardware wallet (Trezor). If you restore a normal wallet (on Wizard) or change the restore height on SettingsInfo, it respects the restore height = 0.

@selsta
Copy link
Collaborator

selsta commented Jun 25, 2021

Yes, that's intentional. Restoring height 0 with hw device sets it to approximately current height.

@rating89us
Copy link
Contributor Author

rating89us commented Jun 25, 2021

But it's not consistent. And it's really strange, because "0" was the default value of restore height field.

Now that the default value of restore height field is blank, don't you think we should change it to respect the "0" entered by the user?

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