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 P2PKH transparent input support to transaction::Builder #107

Merged
merged 10 commits into from
Nov 13, 2019

Conversation

str4d
Copy link
Contributor

@str4d str4d commented Aug 16, 2019

Enabled by the transparent-inputs feature.

@str4d str4d changed the title Add transparent input support to transaction::Builder Add P2PKH transparent input support to transaction::Builder Aug 16, 2019
@@ -19,8 +19,13 @@ pairing = { path = "../pairing" }
rand = "0.7"
rand_core = "0.5"
rand_os = "0.2"
ripemd160 = { version = "0.8", optional = true }
secp256k1 = { version = "=0.15.0", optional = true }
Copy link
Contributor Author

@str4d str4d Aug 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dependency is pinned until rust-bitcoin/rust-secp256k1#150 is resolved, because we don't want to pull rand 0.6 into our dependency tree (and we aren't using the rand feature of secp256k1).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have they fixed this yet? (Issue isn't closed but they seem to have merged some things.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They merged a fix, and then immediately reverted it because they had more point releases to make, and the fix for this requires a minor version bump.

@str4d str4d added this to the v0.2.0 milestone Aug 22, 2019
@str4d
Copy link
Contributor Author

str4d commented Oct 14, 2019

Thanks to @adityapk00 for testing this PR as part of his ZecWallet light client and fixing a bug! This should now work correctly.

@str4d str4d marked this pull request as ready for review October 14, 2019 21:56
@str4d
Copy link
Contributor Author

str4d commented Oct 14, 2019

Hmm, interesting: draft PRs do not show merge conflicts with master in the GitHub UI (I assume because GitHub assumes you'll rebase on master before marking as ready to review). Lesson learned, I'll rebase 😅

@str4d str4d force-pushed the transaction-builder-transparent-inputs branch from e25a2d4 to b479981 Compare October 15, 2019 04:48
@str4d str4d requested review from Eirik0 and ebfull October 15, 2019 04:48
@str4d
Copy link
Contributor Author

str4d commented Oct 15, 2019

Rebased on master to fix merge conflicts.

Copy link
Contributor

@Eirik0 Eirik0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get the following when running cargo test --release:

...
   Compiling zcash_client_backend v0.1.0 (/home/eirik/development/rust_workspace/librustzcash/zcash_client_backend)
warning: use of deprecated item 'rand_os::OsRng': OsRng is now provided by rand_core and rand
   --> zcash_client_backend/src/welding_rig.rs:190:9
    |
190 |     use rand_os::OsRng;
    |         ^^^^^^^^^^^^^^
    |
    = note: #[warn(deprecated)] on by default

warning: use of deprecated item 'rand_os::OsRng': OsRng is now provided by rand_core and rand
   --> zcash_client_backend/src/welding_rig.rs:255:23
    |
255 |         let mut rng = OsRng;
    |                       ^^^^^

warning: use of deprecated item 'rand_os::OsRng': OsRng is now provided by rand_core and rand
   --> zcash_client_backend/src/welding_rig.rs:190:9
    |
190 |     use rand_os::OsRng;
    |         ^^^^^^^^^^^^^^

    Finished release [optimized] target(s) in 2m 19s
     Running target/release/deps/bellman-3e8a46c8cdf6e6ec
...

@str4d
Copy link
Contributor Author

str4d commented Oct 19, 2019

That's due to those slipping in as part of #114 (the warnings were caused by changes to master, that didn't cause merge conflicts with #114 IIRC). They will be fixed in a separate PR.

@@ -19,8 +19,13 @@ pairing = { path = "../pairing" }
rand = "0.7"
rand_core = "0.5"
rand_os = "0.2"
ripemd160 = { version = "0.8", optional = true }
secp256k1 = { version = "=0.15.0", optional = true }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have they fixed this yet? (Issue isn't closed but they seem to have merged some things.)

Copy link
Contributor

@Eirik0 Eirik0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@str4d str4d merged commit 67d700f into zcash:master Nov 13, 2019
@str4d str4d deleted the transaction-builder-transparent-inputs branch November 13, 2019 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants