Skip to content
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

Add Sprout support to TransactionBuilder #3848

Merged
merged 4 commits into from Apr 4, 2019

Conversation

@str4d
Copy link
Contributor

commented Feb 20, 2019

The logic in TransactionBuilder::CreateJSDescriptions() is a combination of the logic used by z_sendmany, z_mergetoaddress and z_shieldcoinbase, in order to support arbitrary numbers of Sprout inputs or outputs.

@str4d str4d requested review from bitcartel and Eirik0 Feb 20, 2019

@str4d str4d added this to Needs Prioritization in Arborist Team via automation Feb 20, 2019

@Eirik0

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2019

@zkbot try

zkbot added a commit that referenced this pull request Feb 20, 2019

Auto merge of #3848 - str4d:transaction-builder-sprout, r=<try>
Add Sprout support to TransactionBuilder

The logic in `TransactionBuilder::CreateJSDescriptions()` is a combination of the logic used by `z_sendmany`, `z_mergetoaddress` and `z_shieldcoinbase`, in order to support arbitrary numbers of Sprout inputs or outputs.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 20, 2019

⌛️ Trying commit 6104625 with merge b483759...

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 20, 2019

☀️ Test successful - pr-try
State: approved= try=True

@mms710 mms710 moved this from Needs Prioritization to Blocked in Arborist Team Feb 21, 2019

@mms710 mms710 moved this from Blocked to PRs That Need Review + Their Associated Issues in Arborist Team Feb 21, 2019

@Eirik0 Eirik0 force-pushed the str4d:transaction-builder-sprout branch from 6104625 to a5ee3ad Feb 26, 2019

@Eirik0 Eirik0 force-pushed the str4d:transaction-builder-sprout branch from 4468515 to 6281cc3 Feb 28, 2019

@mms710 mms710 moved this from PRs That Need Review + Their Associated Issues to In Progress in Arborist Team Mar 14, 2019

@mms710 mms710 moved this from In Progress to In Review in Arborist Team Mar 14, 2019

@Eirik0

Eirik0 approved these changes Mar 15, 2019

Copy link
Contributor

left a comment

ACK on the part done by @str4d

@Eirik0 Eirik0 referenced this pull request Mar 19, 2019

Merged

Sapling migration RPC #3888

@str4d
Copy link
Contributor Author

left a comment

ACK 9e87291 and 6281cc3.

@daira

daira approved these changes Mar 19, 2019

Copy link
Contributor

left a comment

Comments are non-blocking.

Show resolved Hide resolved src/transaction_builder.cpp Outdated
Show resolved Hide resolved src/transaction_builder.cpp Outdated
Show resolved Hide resolved src/transaction_builder.cpp
Show resolved Hide resolved src/transaction_builder.cpp
Show resolved Hide resolved src/transaction_builder.cpp

@daira daira self-assigned this Mar 28, 2019

@daira

daira approved these changes Apr 1, 2019

Copy link
Contributor

left a comment

ut(ACK+cov)

Show resolved Hide resolved src/transaction_builder.cpp
Show resolved Hide resolved src/transaction_builder.cpp
@daira

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

This now meets the minimum 2 reviews, but there are potential loss-of-funds and privacy issues if we get it wrong, so another review would be good.

@Eirik0 Eirik0 requested review from LarryRuane and mdr0id Apr 1, 2019

@daira daira removed their assignment Apr 2, 2019

@bitcartel
Copy link
Contributor

left a comment

If (when?) we test z_sendmany using TransactionBuilder for Sprout based sends, the existing Sprout shielded qa tests should detect any issues or changes in behaviour.

@mms710 mms710 moved this from In Review to Merge Queue in Arborist Team Apr 4, 2019

@Eirik0 Eirik0 closed this Apr 4, 2019

Arborist Team automation moved this from Merge Queue to Released (Merged in Master) Apr 4, 2019

@Eirik0 Eirik0 reopened this Apr 4, 2019

Arborist Team automation moved this from Released (Merged in Master) to Needs Prioritization Apr 4, 2019

@Eirik0

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Apr 4, 2019

📌 Commit ee9ff2c has been approved by Eirik0

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Apr 4, 2019

⌛️ Testing commit ee9ff2c with merge 926fd0c...

zkbot added a commit that referenced this pull request Apr 4, 2019

Auto merge of #3848 - str4d:transaction-builder-sprout, r=Eirik0
Add Sprout support to TransactionBuilder

The logic in `TransactionBuilder::CreateJSDescriptions()` is a combination of the logic used by `z_sendmany`, `z_mergetoaddress` and `z_shieldcoinbase`, in order to support arbitrary numbers of Sprout inputs or outputs.

@Eirik0 Eirik0 moved this from Needs Prioritization to Sprint Backlog in Arborist Team Apr 4, 2019

@Eirik0 Eirik0 moved this from Sprint Backlog to In Progress in Arborist Team Apr 4, 2019

@Eirik0 Eirik0 moved this from In Progress to Merge Queue in Arborist Team Apr 4, 2019

@Eirik0 Eirik0 added this to the v2.0.5 milestone Apr 4, 2019

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Apr 4, 2019

☀️ Test successful - pr-merge
Approved by: Eirik0
Pushing 926fd0c to master...

@zkbot zkbot merged commit ee9ff2c into zcash:master Apr 4, 2019

1 check passed

homu Test successful
Details

Arborist Team automation moved this from Merge Queue to Released (Merged in Master) Apr 4, 2019

@daira daira modified the milestones: v2.0.5, v2.0.4 Apr 16, 2019

@mms710 mms710 modified the milestones: v2.0.4, v2.0.5 Apr 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.