-
Notifications
You must be signed in to change notification settings - Fork 12
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
Allow getting BTC address for an account when requesting a NIM address #437
Conversation
7cf2627
to
7b09ac1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simple, effective. Found nothing wrong. Nice 👍
- Fix import order to avoid webpack warnings about conflicting import order between chunks choose-address, sign-btc-transaction, sign-btc-transaction-ledger, activate-btc, swap, swap-ledger, refund-swap-ledger, add-account, add-ledger - Ensure parsing of ChooseAddressRequest into expected types - Use State constant - await WalletStore.put - Fix address index off-by-one error
76abcf5
to
990b0be
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed some fixes and added two suggestions and two comments in the vue-components change.
// const startIndex = Math.max(Math.min( | ||
// walletInfo.btcAddresses.external.findIndex((addressInfo) => !addressInfo.used), | ||
// walletInfo.btcAddresses.internal.findIndex((addressInfo) => !addressInfo.used), | ||
// ), 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not scanning all addresses again would indeed be good.
The current implementation tries to fill gaps between used addresses, which is good and wouldn't be accomplished by just using the start index of the last used address.
However, scanning could start at the last known, gap-less used address for internal and external addresses or even better just re-scan only the known, unused addresses for whether they have been used in the meantime and start scanning at the last known address only if then still required.
Allow the
ChooseAddress
request to also request a BTC address for the account of which the NIM address was selected. It does not enforce selecting an account that supports BTC, so thebtcAddress
in the result can still beundefined
. I have additionally updated theAccountSelector
component in vue-components a bit to display a "BTC" label next to BTC-enabled account labels, if a BTC address is expected in the result.Additionally, I have made all filtering
AccountSelector
props available as request options, to filter the list of selectable addresses (which makes sense in the CPL case, where contract addresses should not be able to be selected, as they cannot receive NIM).