Conversation
|
~~Closing this, it has a custom Go descriptor checksum implementation, while #3909 uses the descriptors-go. ~~ Re-opened, for now it seems better to use the port than to pull in a precompiled .wasm binary. |
benma
force-pushed
the
descriptors
branch
2 times, most recently
from
5742988 to
ad11a48
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad11a48996
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| for _, cfg := range info.SigningConfigurations { | ||
| signingConfig := signingConfigurationInfo{} | ||
| if cfg.BitcoinSimple != nil { |
There was a problem hiding this comment.
getAccountInfo now unconditionally dereferences each cfg in info.SigningConfigurations, but Ethereum accounts can expose a nil entry before initialization (backend/coins/eth/account.go returns []*signing.Configuration{account.signingConfiguration} while account.signingConfiguration starts nil). In that state, calling /account/{code}/info will hit a nil-pointer panic here and return an API error instead of account info, so this loop should handle cfg == nil explicitly.
Useful? React with 👍 / 👎.
|
|
||
| // Descriptor returns a descriptor for this configuration in the form | ||
| // SCRIPT([fingerprint/path]xpub-or-tpub/<0;1>/*)#checksum. | ||
| func (configuration *BitcoinSimple) Descriptor(net *chaincfg.Params) (string, error) { |
There was a problem hiding this comment.
Why do we need to specify the net? Isn't it embedded in the xpub?
There was a problem hiding this comment.
Good question! I am not 100% sure if xpubs of testnet accounts were always serialized as tpub... in accounts.json, so this makes sure it is shown in the correct format.
We also explicitly set it for the normal xpub shown in the account info:
|
|
||
| originPath := strings.TrimPrefix(configuration.KeyInfo.AbsoluteKeypath.Encode(), "m/") | ||
| keyOrigin := hex.EncodeToString(configuration.KeyInfo.RootFingerprint) | ||
| if originPath != "" { |
There was a problem hiding this comment.
Does it make sense to go on without a keypath? Or should we error?
There was a problem hiding this comment.
I would say this function should not be concerned with that, it is technically a valid descriptor even with an empty path too. Probably never going to be used like this by the app though, so it's a tossup 🤷♂️ any strong preference?
There was a problem hiding this comment.
Slight preference for returning an error, since the whole point is to include keypaths info. But no strong opinion.
| "label": "Account info", | ||
| "scriptType": "Script type", | ||
| "taproot": "The Taproot extended public key is not shown to protect from accidental misuse, as raw formats for this script type are not widely supported. If you want to view this key, connect your device to wallet software that supports the output descriptor format.", | ||
| "taproot": "To import this account into another wallet, make sure it supports descriptors. Copy the <strong>full descriptor</strong>.\n<strong>Do not use only the xpub</strong>, or the wallet may import the account with the wrong script type.", |
There was a problem hiding this comment.
Is this an issue for taproot only?
There was a problem hiding this comment.
I was going to say yes, because for the other types, the xpub is shown as ypub or zpub, so unambiguous. But you made me aware that users might attempt to copy only the xpub part inside the descriptor, which would be a big problem actually.
I think I'll show that info for all types, and right above the descriptor to mitigate that.
There was a problem hiding this comment.
I like having the banner also for native segwit, but now hiding the xpub only for taproot looks almost like a bug, imo. Would it be ok to keep the banner and the xpub in both places?
Another small thing: on native segwit the xpub is shown in the 'zpub' format when alone, but in the 'xpub' format inside the descriptor. Is it intended?
There was a problem hiding this comment.
I would not show the xpub in Taproot even though it looks inconsistent, as it led to a ton of issues with stuck funds that could not be recovered with a BitBox - which was the reason we removed it in the first place. Better not risk it.
zpub alone and xpub in descriptor is intended, zpub is Electrum format and encodes that it is p2wpkh (adopted by many other wallets), while descriptors are explicit about what the script is and only use xpub/tpub.
|
One more question: does it really make sense to both port the descriptor checksum code from bitcoin core and also import a new dep only to double check the result in the test? Can't we just use one of them? |
I had an alternative PR in #3909 that only uses the new dep, but @NickeZ and I discussed a bit the problem of pulling in an unvetted WASM binary for production code, and opted for not pulling it in for now. If you prefer, I could remove the dep also for the tests, and instead inline a script to generate some test vectors? |
benma
force-pushed
the
descriptors
branch
4 times, most recently
from
e7b00e5 to
cc573e8
Compare
That would be nice! But I don't know if it's worth the effort. Feel free to keep as is, if you prefer 👍 |
Returns the account descriptor that can be imported in third party wallets that support descriptors. See https://github.com/bitcoin/bitcoin/blob/master/doc/descriptors.md. This is especially important to be able to export/import Taproot accounts, where the `xpub` alone can easily be confused for a legacy p2pkh account. The descriptor contains the complete derivation info.
3 participants
Also reworded the warning banner for Taproot: