Master to staging-revamp-settings #2142
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This fixes 'handshake must come first' error, the sdcard check should only happen after the bitbox02 is paired.
This adds notes to utxo struct, to be displayed when using coin control feature.
We would like to update nodejs to v18.16.0, which requires libc6 with a higher version than that available on ubuntu 18.04 (which reached end of life on 30/04/2023). This commit updates shiftcrypto/qt5 and shiftcripto/bitbox-wallet-app docker images to be based upon ubuntu 20.04 and allows updating nodejs.
Previous gradle version 3.5.4 was pretty old an deprecated: this updates gradle to v8.0 and android gradle plugin to v8.0.1. The update has been done through Android Studio interface and caused all the changes listed in this commit.
Android Gradle Plugin v8 (updated in the previous commit) requires jdk-17.
Android Studio problem tab was listing several errors and warnings across the files. This fixes all the errors and some warnings. There are still a lot of warnings around, but they looks like minor stuff so I didn't touch them for now.
This commit adds typed channelHashChanged, statusChanged and attestationCheckDone to devicessync and uses it in bitbox02.tsx and bitbox02bootloader.tsx
Since setup uses fullscreen View component the sidebar is always hidden anyway, but we still had logic that checked if sidebar has to be hidden or visible. Removed code introduced in ea756e0 Also remove forceCollapsed sidebarstate in waiting.tsx component, as this seem to not be used anymore. The only remaining component that needs shared sidebar is not bitbox01 setup.
syncAddressesCount is already used in accounts overview, but was not in account summary. With this change syncAddressesCount is now used in account summary, next step is to convert the other apiWebsocket subscriptions in accocunt summary to statusChanged and syncdone.
Internally syncdone and statuschanged use the legacy events, but from api consumer point it doesn't matter. Moved to accountsync as that makes more sense.
Part of moving all subscriptions to typed api. There is still untyped device signProgress and signConfirm subscriptions to be typed in a later commit. Also changed code prop to mandatory, send should always have a code prop, which is the account to send from.
Accountsummary may not have all accounts on mount and needs to
re-subscribe on componentDidUpdate.
Simplifying the code a bit by removing map, return and a Promise.all
that did nothing, seems to be a leftover.
To test this delete account cache before servewallet:
rm -rf appfolder.dev/cache/account-* && make servewallet
Next step is changing apiWebsocket to syncdone and statusChanged.
And also fixes typo subscibeLegacy -> subscribeLegacy
Added BitBox01 serial number for customer support purposes.
The error returned by handlers is not propagated to the frontend in
production. For errors that should be displayed to the user, we
currently use the `{ success: false }` pattern, sometimes with an
error message and/or error code.
It has been a frequent confusion that one can simply return an error
from the handler. This commit adds a non-error version of the handler
function, with the goal that all handlers eventually use this one, so
errors are properly handled in all cases.
Moved the 3 success views to success.tsx. Only the create-wallet success has the nice success animation. The other 2 success views show a list with security notices. This could be improved in the future so each has the nice animated checkmark and different success messages. This is the first commit moving each setup step to its own component so that it is easier in the future to make changes to the setup.
Moved set-password setup step to its own component, there are two versions. One is normal set-password, the second one is used when restoring-from-sdcard and contains additional information about which backup name and hash.
Moved the list of security notices, that have to each be checked when creating a new wallet, to its own component. The state of each checkbox does not have to be in the parent (bitbox02.tsx) as it is only used to disable the continue button inside this view. Note there are 2 similar list with security notices, but without checkboxes, that are currently used as restore success view. It probably makes sense to move the other 2 lists into this component as well and add an additional success step for restore in the future.
creatingBackup was only used to block the continue button on the checklist with all warnings regarding the backup. Simplified by setting all checkboxes to false once continue is clicked so that the continue button is disabled after clicking, the view is anyway taken over by either confirm date or insert sdcard prompt.
Renamed the follwing functions for better readability: - backupOnBeforeRestore to onSelectBackup - backupOnAfterRestore to onRestoreBackup
Moved the view that lists all backups into its own view for better readability.
As setConfig merges the config object with current config, passing backend is not needed anymore. It was needed before when using setconfig directly so backend configs were not lost.
Add support for Zloty (PLN), this is in the list of currencies supported by CoinGecko: - https://api.coingecko.com/api/v3/simple/supported_vs_currencies
Add support for Czech Crown (CZK), this is in the list of currencies supported by CoinGecko: - https://api.coingecko.com/api/v3/simple/supported_vs_currencies
|
@thisconnect not sure how you usually fix merge conflicts on (protected) staging branch. For now I made a branch that fixed the merged conflicts of master into staging-revamp-settings (this PR). And I made a PR to merge that branch into this branch. PR is here: #2143 . EDIT: Probably makes sense to wait for #2141 to get merged first actually :) Then, if we merge #2143, we can change this PR from draft to ready for review and I believe merge this one too, then merge #2129. 😄 please lmk wyt. |
|
yeah exactly, I am just not sure if I can review this. should i just approve your PR or can you point me to the places where there was a conflict. let me know if it makes sense to even review.
|
|
merged!
|
I did a locize pull before a locize push in 8dbaab3, accidentally removing keys.
Yes sounds good. The conflicts happened in For For So once youve reviewed those, you can approve #2143 and I'll merge it. Then I can merge this one, and finally #2129 . 🤞 lmk wyt! |
|
#2143 merged! Feel free to approve this one too cc @thisconnect 🙏 |
An account could be Initialized() after it was already Closed() if they run at the same time. In ETH, the isClosed check was missing altogether, but even in btc, there was a race condition. We need to use the same lock for both. The `WaitSynchronized()` in the Close function of the ETH account also seems unneccesary and if it blocks the Close indefinitely, the DB cannot be closed and the an account with the same DB cannot be initialized (e.g. in `ReinitializeAccounts()`). Not sure why it was added, the commit that introduced it had no explanation. I ran into these issues while working on unit tests for accounts sync.
The `account.Config().Config` is a pointer to the persisted account stored in `backend.config.accountsConfig.Accounts`. By making the the slice entry also a pointer, we can make sure that the `account.Config().Config` always points to the persisted config. Then, we can simply modify the persisted config to also modify the loaded config. Before that, the same was true but not deterministically: the stored account config and the loaded config would point to the same memory location, but only as long as the persisted account was not moved, which can happen when the slice is extended (so the slice and all contents is moved to a new location while growing the slice). If this happens, then the loaded config would become a shallow copy, which is error prone: modifying a primitive field would modify a copy, while modifying a pointer field (e.g. the signing configuration) would modify both the loaded and the persisted config. The alternative to this commit would have been to instead make a deep copy of the persisted config when loading it. It seemed to me like just sharing the same memory here is simpler overall.
By being able to mock the accounts, it is much easier to write unit tests that rely on specific acocunt behavior. For example, in accounts discovery we check if `.Transactions()` returns an empty or non empty list to decide when to stop scanning accounts. Without mocking the account, we'd have to connect to mocked Electrum/Etherscan servers, which is much harder to do and maintain.
We want to track if an account has been used (i.e. has a transaction history). This is info is required to decide if the next account can be created, according to BIP39.
In order to scan accounts until we find an empty account, we always need to scan one account ahead of what is visible to the user. For this, we introduce a flag "hiddenBecauseUnused", which is false by default and only true if an account is added to be scanned in the background. It is not shown to the user at all, until either: - it is scanned and has a transaction history - the user adds a new account in the settings - this activtes the hidden account if it was automatically added before. Such hidden accounts are persisted for the same reason the regular accounts are persisted: creating the entry requires the xpubs, which requires a keystore (BitBox02), which is slow. By persisting it we do not need to query the keystore for multiple xpubs everytime for the hidden account, but just once. The part where we add the hidden account automatically follows in the next commit.
We implement BIP-44 accounts scanning, meaning: accounts per coin are scanned until we find an unused account. See the docstring of `maybeAddHiddenUnusedAccounts()` for details, especially how and why we deviate from BIP-44.
`Accounts()` only held the read lock but modified the accounts, which could lead to a race condition.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.