Replace `Backend.{createToAddress, shieldToAddress}` with proposal-based methods by str4d · Pull Request #1344 · zcash/zcash-android-wallet-sdk

str4d · 2024-01-08T23:05:39Z

Migrates to the latest in-progress revision of the Zcash Rust crates, so this is not suitable for inclusion in an SDK release yet.

Closes #1338.

Note
This code review checklist is intended to serve as a starting point for the author and reviewer, although it may not be appropriate for all types of changes (e.g. fixing a spelling typo in documentation). For more in-depth discussion of how we think about code review, please see Code Review Guidelines.

Author

Self-review your own code in GitHub's web interface¹
Add automated tests as appropriate
Update the manual tests² as appropriate
Check the code coverage³ report for the automated tests
Update documentation as appropriate (e.g README.md, Architecture.md, etc.)
Run the demo app and try the changes
Pull in the latest changes from the main branch and squash your commits before assigning a reviewer⁴

Reviewer

Check the code with the Code Review Guidelines checklist
Perform an ad hoc review⁵
Review the automated tests
Review the manual tests
Review the documentation, README.md, Architecture.md, etc. as appropriate
Run the demo app and try the changes⁶

Code often looks different when reviewing the diff in a browser, making it easier to spot potential bugs. ↩
While we aim for automated testing of the SDK, some aspects require manual testing. If you had to manually test
something during development of this pull request, write those steps down. ↩
While we are not looking for perfect coverage, the tool can point out potential cases that have been missed. Code coverage can be generated with: ./gradlew check for Kotlin modules and ./gradlew connectedCheck -PIS_ANDROID_INSTRUMENTATION_TEST_COVERAGE_ENABLED=true for Android modules. ↩
Having your code up to date and squashed will make it easier for others to review. Use best judgement when squashing commits, as some changes (such as refactoring) might be easier to review as a separate commit. ↩
In addition to a first pass using the code review guidelines, do a second pass using your best judgement and experience which may identify additional questions or comments. Research shows that code review is most effective when done in multiple passes, where reviewers look for different things through each pass. ↩
While the CI server runs the demo app to look for build failures or crashes, humans running the demo app are
more likely to notice unexpected log messages, UI inconsistencies, or bad output data. Perform this step last, after verifying the code changes are safe to run locally. ↩

str4d · 2024-01-08T23:17:11Z

+    companion object {
+        fun parse(encoded: ByteArray): ProposalUnsafe {
+            val inner = Proposal.parseFrom(encoded)
+            return ProposalUnsafe(inner)


Despite this being called ProposalUnsafe, I expect we'll do most validation in this class because we have access to the parsed protobuf (which can't be used outside backend-lib without requiring sdk-lib to depend on the protobuf library, and I figure it's better to keep it being a backend-internal detail). The wrapping Proposal in sdk-lib is wrapping the exposed types here in safe API types like Zatoshi.

Yes, that is the way we do it with the other models as well.

nuttycom

utACK with nonblocking nits. Some of these, on further reflection, are just design issues that should probably be cleaned up separately; the one that I'd like clarification on is the UnsafeProposal vs Proposal situation and the meaning of the parse method on Proposal; please document those types/methods more completely.

nuttycom · 2024-01-09T15:58:37Z

+import cash.z.wallet.sdk.ffi.ProposalOuterClass.FeeRule
+import cash.z.wallet.sdk.ffi.ProposalOuterClass.Proposal
+
+class ProposalUnsafe(


Does this need to be documented, or is it not exposed to the SDK user?

I don't intend for it to be exposed to the SDK user, but IDK if I succeeded within Kotlin.

It's not ideal, as the class is still reachable from clients, but this is how we used to with, e.g., jni models too. We're helping ourselves with the internal in the package.

nuttycom · 2024-01-09T16:28:51Z

            .map(|account| {
-                let account_id = AccountId::from(account);
+                let account_id =
+                    AccountId::try_from(account).map_err(|_| format_err!("Invalid account ID"))?;


Since accounts has been cast from a jint, it cannot be more than 2^31-1, so this error should never occur. This could also be an expect.

nuttycom · 2024-01-09T16:31:10Z

+fn account_id_from_jint(account: jint) -> Result<AccountId, failure::Error> {
+    u32::try_from(account)
+        .map_err(|_| ())
+        .and_then(|id| AccountId::try_from(id).map_err(|_| ()))


This try_from should never be able to fail, because the u32::try_from will have ensured that the resulting u32 falls into the valid range of account IDs. I think that using map and expect here would actually be appropriate, because if the inner try_from fails that should indicate that something is crashingly wrong.

nuttycom · 2024-01-09T16:47:21Z

-    output_params: JString<'_>,
    network_id: jint,
    use_zip317_fees: jboolean,
 ) -> jbyteArray {


nit: It might be nice to have a type alias for jbyteArray that indicates that this is a serialized proposal protobuf. The fact that the return type didn't change was a little disorienting.

nuttycom

Now that [zcash/librustzcash#1060] has merged, please update proposal.proto before this merges.

str4d · 2024-01-10T20:17:25Z

Rebased on main.

str4d · 2024-01-10T21:34:58Z

Force-pushed to address review comments.

nuttycom

proposal.proto is still the older version, not what's on current ][zcash/librustzcash@main]

str4d · 2024-01-11T02:53:03Z

Force-pushed to update proposal.proto (I had done so locally, and then stashed it to split the changes across the separate commits and forgot to unstash it).

nuttycom

utACK b3cfad5

nuttycom · 2024-01-11T22:19:23Z

There appear to be a bunch of linting errors being reported.

HonzaR

Thanks for these changes. I just pushed a few minor commits.

IIUC our intention here is to have a two-step transaction submitting? I.e. 1. get transaction proposal, 2. submit transition.

To achieve this, we need to update the Synchronizer's API to provide the new propose API methods, which would call related missing propose methods in the txManager and tweak the existing sendToAddress and shieldToAddress Is that correct?

HonzaR · 2024-01-17T17:44:19Z

+    companion object {
+        fun parse(encoded: ByteArray): ProposalUnsafe {
+            val inner = Proposal.parseFrom(encoded)
+            return ProposalUnsafe(inner)


Yes, that is the way we do it with the other models as well.

HonzaR · 2024-01-17T17:46:09Z

+import cash.z.wallet.sdk.ffi.ProposalOuterClass.FeeRule
+import cash.z.wallet.sdk.ffi.ProposalOuterClass.Proposal
+
+class ProposalUnsafe(


It's not ideal, as the class is still reachable from clients, but this is how we used to with, e.g., jni models too. We're helping ourselves with the internal in the package.

str4d · 2024-01-17T21:04:19Z

IIUC our intention here is to have a two-step transaction submitting? I.e. 1. get transaction proposal, 2. submit transition.

To achieve this, we need to update the Synchronizer's API to provide the new propose API methods, which would call related missing propose methods in the txManager and tweak the existing sendToAddress and shieldToAddress Is that correct

It's not strictly necessary, but it would make the most sense to do this. If we don't make this change then we end up calling the propose API twice, once to get the fee and then again during transaction creation, and the second time the fee could technically change. If we expose a Proposal type to SDK users that has the fee API on it, then wallets can use that to display and confirm fee, and then pass the Proposal back to the SDK for transaction creation.

Eventually we will want to enable the SDK user to request that proofs start being created in the background at the time the Proposal is created, so then if they pass that Proposal back the SDK can look up that work and create the final transaction more quickly. So if there are changes to how the public two-step API works that would be necessary to enable that background caching and subsequent association, that don't make implementing this change harder now, then we might as well incorporate it into the new API.

str4d · 2024-01-19T02:00:21Z

Rebased on #1357 and now targeting the 2.1.0 feature branch. I will rebase again once that merges.

str4d · 2024-01-22T17:25:24Z

Rebased on feature-2.1.0 now that #1357 is merged.

…sed methods Closes #1338.

- This copies the pattern from lightwallet-client-lib module where we set the package for the generated classes - It adds internal to the path, to be explicit about not exposing it out of the backend module

str4d · 2024-01-22T22:03:52Z

Force-pushed to fix lints.

HonzaR

Additionally, I tested it with Zashi via included builds, and it works as expected.

str4d commented Jan 8, 2024

View reviewed changes

Comment thread backend-lib/build.gradle.kts Outdated

str4d commented Jan 8, 2024

View reviewed changes

str4d requested a review from HonzaR January 8, 2024 23:28

nuttycom approved these changes Jan 9, 2024

View reviewed changes

nuttycom requested changes Jan 9, 2024

View reviewed changes

str4d force-pushed the 1338-proposal-ffi branch from e503d16 to c00b0de Compare January 10, 2024 20:17

str4d force-pushed the 1338-proposal-ffi branch from c00b0de to 0a7cf96 Compare January 10, 2024 21:34

nuttycom requested changes Jan 10, 2024

View reviewed changes

str4d force-pushed the 1338-proposal-ffi branch from 0a7cf96 to b3cfad5 Compare January 11, 2024 02:52

nuttycom approved these changes Jan 11, 2024

View reviewed changes

HonzaR reviewed Jan 17, 2024

View reviewed changes

str4d force-pushed the 1338-proposal-ffi branch from b6656fe to 8f5b3c2 Compare January 19, 2024 01:58

str4d changed the base branch from main to feature-2.1.0 January 19, 2024 01:58

str4d force-pushed the 1338-proposal-ffi branch from 8f5b3c2 to 14da738 Compare January 22, 2024 17:24

str4d force-pushed the 1338-proposal-ffi branch from 14da738 to 457984e Compare January 22, 2024 17:39

str4d and others added 2 commits January 22, 2024 22:02

Replace Backend.{createToAddress, shieldToAddress} with proposal-ba…

914d26c

…sed methods Closes #1338.

Explicit generated location

f3a56d1

- This copies the pattern from lightwallet-client-lib module where we set the package for the generated classes - It adds internal to the path, to be explicit about not exposing it out of the backend module

str4d force-pushed the 1338-proposal-ffi branch from 457984e to f3a56d1 Compare January 22, 2024 22:03

HonzaR mentioned this pull request Jan 23, 2024

Expose the transaction proposal in a way that enables querying its fee #1359

Closed

HonzaR approved these changes Jan 23, 2024

View reviewed changes

str4d merged commit 1bcd794 into feature-2.1.0 Jan 23, 2024

str4d deleted the 1338-proposal-ffi branch January 23, 2024 12:56

Conversation

str4d commented Jan 8, 2024 • edited by HonzaR Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Author

Reviewer

Footnotes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nuttycom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nuttycom left a comment

Choose a reason for hiding this comment

Uh oh!

str4d commented Jan 10, 2024

Uh oh!

str4d commented Jan 10, 2024

Uh oh!

nuttycom left a comment

Choose a reason for hiding this comment

Uh oh!

str4d commented Jan 11, 2024

Uh oh!

nuttycom left a comment

Choose a reason for hiding this comment

Uh oh!

nuttycom commented Jan 11, 2024

Uh oh!

HonzaR left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

str4d commented Jan 17, 2024

Uh oh!

str4d commented Jan 19, 2024

Uh oh!

str4d commented Jan 22, 2024

Uh oh!

str4d commented Jan 22, 2024

Uh oh!

HonzaR left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

str4d commented Jan 8, 2024 •

edited by HonzaR

Loading