-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
BIP 329: Add spendable
state to outputs
#1452
Conversation
spendable
state to outputs
LGTM. As you suggest I'd like to see #1412 merged first, but I've had enough requests for a spendable (or frozen) status in BIP329 that I think it's worth the slight scope creep. I don't believe this indicator fits well into any other specification, so having it in the wallet labels format makes the most sense to me as a wallet developer. |
Would you prefer that I expanded this PR to include copy changes to make it clear it's not just labels anymore? Or would you prefer it to stay as-is besides the new key-value pair? Happy to do the leg work expanding copy if you'd prefer it that way. |
I think it's worth adding a line to the motivation yes - something along the lines of "In addition, many wallets allow unspent outputs to be frozen or made unspendable within the wallet. Since this wallet-related metadata is similar to labels and not captured elsewhere, it is also included in this format." |
Added with minor formatting tweaks in ab2f2b1. Thanks for the prompt feedback! |
Noting that #1412 has now been merged, so this PR can be rebased. |
Rebased, all set for merge now! |
Working on this currently, and I was wondering about this default:
Would it not be better for the importing wallet to simply not try to change the spendable status if no |
There's a further nuance too - should an export wish to set the Each JSON object must only contain key/value pairs, defined as follows: [table of properties] [property reference] Each JSON object must contain both type and ref properties. The label, origin and spendable properties are optional. If the label or spendable properties are omitted, the importing wallet should not alter these values. The origin property should only appear where type is tx, and the spendable property only where type is output. |
Agreed on both fronts, I hadn't considered that edge case for the spendable state, nor the JSON overwrites! Updating now. |
LGTM. Implemented in Sparrow in sparrowwallet/sparrow@dbbeaf6. |
This PR adds a
spendable
state to outputs in BIP 329, allowing wallets to properly export and import the spendable state of outputs.This allows wallets to prevent users accidentally spending funds they previously marked as unspendable in wallets like Sparrow or Samourai that support coin control.
We plan to implement the usage of BIP 329 into Envoy shortly, and wanted to ensure that our users have a way to transfer spendable state in and out of the wallet and to other BIP 329-respecting wallets without issues, thus the PR.
Note: this does expand the scope of this BIP very slightly, almost making it a wallet "metadata" BIP instead of purely label-related. I'm happy to expand the PR accordingly by updating the title and copy to reflect that slight scope shift if desired but wanted to start the conversation off with the minimum changes necessary.
This PR currently would conflict with #1412, but I will rebase once that PR is merged to accommodate it and then can squash commits.