[Refactor] SaplingOperation: support for multisig, cold-staking, and OP_RETURN outputs #1967
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
3 tasks
random-zebra
force-pushed
the
202011-saplingOperation-recipients
branch
from
1f4da22 to
67130a6
Compare
Author
|
Rebased. |
furszy
previously approved these changes
Nov 15, 2020
furszy
left a comment
furszy
left a comment
There was a problem hiding this comment.
Overall looking great, ACK 67130a65 .
The only minor thing that would add in every SendManyRecipient constructor is the nullopt member initialization for the member that is not used. Example, if it's being feed by a shielded addr, set the transparent optional to nullopt and vice versa.
Save them as either CTxOut (transparent outputs) or a tuple with sapling address, amount and memo (shielded outputs), instead of strings. This saves a redundant Encoding/Decoding operation while offering more flexibility (allowing for all possible kinds of transparent outputs).
random-zebra
force-pushed
the
202011-saplingOperation-recipients
branch
from
67130a6 to
cf9d4df
Compare
Author
|
Rebased on master and added initializer lists. |
furszy
added a commit
that referenced
this pull request
Nov 18, 2020
467b0d8 [Tests] Check delegations with sapling transactions (random-zebra) d412525 [RPC] Add fUseShielded parameter to delegatestake/rawdelegatestake (random-zebra) 847d278 [Trivial] Missing newline in delegatestake (random-zebra) 7440709 [Validation] Don't reject sapling txes with P2CS outputs (random-zebra) Pull request description: Enable pay-to-cold-staking outputs in shielded transactions. Add the option in `delegatestake`/`rawdelegatestake` to spend shielded funds directly to P2CS (without having to unshield them first). Add functional test. Based on top of - [x] #1964 - [x] #1967 - [x] #1969 Starts with `[Validation] Don't reject sapling txes with P2CS outputs` (461d4ea69ea21ea639ae639d4248b4ef28312429) ACKs for top commit: furszy: Looking good, ACK 467b0d8 Fuzzbawls: ACK 467b0d8 Tree-SHA512: 88752fe1550def605b90cc894d0666f5c37dd8f075566e976c8fbfaa6e088ee0c1bb028e1885411b739781fda6ebac12a5c99fc19a06f453e594441ee1e1b599
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Refactor
SaplingOperation, abstracting the separation between shielded and transparent outputs insideSendManyRecipient.Instead of using strings for the internal representation, save the recipients as either
CTxOut(for transparent outputs) or a tuple comprising SaplingAddress, Amount and Memo (for sapling outputs).This has the following pros:
CTxOutcan be any kind of transaction output, we can easily integrate other use-cases.For example, in this PR we add support for multisig, P2CS and op_return outputs.