Expose APIs for working with transaction proposals #1382

str4d · 2024-02-21T01:02:36Z

Closes #1343.

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: Did you review your own code in GitHub's web interface? Code often looks different when reviewing the diff in a browser, making it easier to spot potential bugs.
Automated tests: Did you add appropriate automated tests for any code changes?
Code coverage: Did you check the code coverage report for the automated tests? While we are not looking for perfect coverage, the tool can point out potential cases that have been missed.
Documentation: Did you update Docs as appropiate? (E.g README.md, etc.)
Run the app: Did you run the app and try the changes?
Did you provide Screenshots of what the App looks like before and after your changes as part of the description of this PR? (only applicable to UI Changes)
Rebase and squash: Did you pull in the latest changes from the main branch and squash your commits before assigning a reviewer? 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.

Reviewer

Checklist review: Did you go through the code with the Code Review Guidelines checklist?
Ad hoc review: Did you perform an ad hoc review? 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.
Automated tests: Did you review the automated tests?
Manual tests: Did you review the manual tests?You will find manual testing guidelines under our manual testing section
How is Code Coverage affected by this PR? We encourage you to compare coverage befor and after your changes and when possible, leave it in a better place. Learn More...
Documentation: Did you review Docs, README.md, LICENSE.md, and Architecture.md as appropriate?
Run the app: Did you run the app and try the changes? While the CI server runs the app to look for build failures or crashes, humans running the app are more likely to notice unexpected log messages, UI inconsistencies, or bad output data.

Sources/ZcashLightClientKit/Synchronizer/SDKSynchronizer.swift

nuttycom

utACK with questions that we should probably create follow-up issues to address.

Sources/ZcashLightClientKit/Synchronizer.swift

Sources/ZcashLightClientKit/Synchronizer/SDKSynchronizer.swift

Sources/ZcashLightClientKit/Transaction/TransactionEncoder.swift

nuttycom · 2024-02-22T01:54:07Z

Sources/ZcashLightClientKit/Transaction/WalletTransactionEncoder.swift

            usk: spendingKey
        )

-        return txId
+        logger.debug("transaction id: \(txId)")
+        return [try await repository.find(rawID: txId)]


Does repository.find throw if it cannot find the txid? I would think that ideally that would return an optional result, so that "not found" is distinguished from "error" (though in this particular case you'd want to throw an error anyway, so it's nonblocking for now.)

We shouldn't encounter this case (successful wallet DB lookup that returns no result) because if we reach here, the transaction was successfully created by the Rust backend (and didn't itself result in an exception). Any exception thrown here would instead be due to database access failures, which are indeed exceptions.

Sources/ZcashLightClientKit/Synchronizer/SDKSynchronizer.swift

Sources/ZcashLightClientKit/Synchronizer.swift

Sources/ZcashLightClientKit/Transaction/TransactionEncoder.swift

daira

utACK modulo comments.

str4d · 2024-02-27T16:05:29Z

Force-pushed to address review comments.

Sources/ZcashLightClientKit/Synchronizer/CombineSDKSynchronizer.swift

Sources/ZcashLightClientKit/Transaction/TransactionEncoder.swift

pacu · 2024-02-29T16:42:03Z

Sources/ZcashLightClientKit/Model/Proposal.swift

+    }
+}
+
+public extension Proposal {


I would advise not to expose this publicly.

If you have a suggestion for how to avoid exposing this publicly, I'm all ears. The constraint here is Zashi iOS using TCA, which needs to be able to construct mock data to be returned from its mock implementation of the Synchronizer protocol for use in e.g. preview of UIs (so we can't just return something that would be an error case).

NOTE: I don't know which things you've tried and which ones you haven't, but there are a few things I can think from the top of my head. I promise I'll look this deeper shortly. I might be still lacking context and this could be somewhat inaccurate or insufficient.

A. "Uncle bob's Clean code" approach

Treat the whole SDK as an external dependency under the principle of Bob Martin's clean code architecture. Look at the problem as you were Unstoppable or EDGE and you don't control the dependency. Wrap around it and have a shim that you Do control and use that instead.

B. Pointfree.co / TCA approach.

Make proposal on the SDK Protocol. The SDK returns an implementation of it. Then use a protocol witness or a mock implementation on Zashi to provide the mocked version that you need.

A and B

Make a protocol on Zashi for what the app expects of a proposal. This propocol contains a throwing toProposal() function.
Make an Adapter of the SDK's Proposal type that conforms to Zashi's.
Make a Mock implementation for testing on Zashi that throws whenever toProposal() is invoked. If by mistake this ever happens and a mocked proposal is sent in production you can see the error and flag it as critical in Crash reporting platform.

Or better, the testing code is never available in outside of the testing target.

Closes #1204.

- Returns `null` when there are no funds to shield or the shielding threshold is not met. - Throws an exception if there are funds to shield in more than one transparent receiver within the account. - Has an optional parameter for specifying which transparent receiver to shield funds from. This commit only alters the API to support the above; the functional changes require modifying the FFI and Rust backend, which will happen in a separate commit.

Closes #1186.

str4d · 2024-03-06T02:55:20Z

Force-pushed to clean up the commits.

Includes: - Multi-step transaction proposals. - Changes to support `Synchronizer.proposeShielding` API changes.

LukasKorba · 2024-03-06T18:34:43Z

I tested all 5 APIs while working on adoption of this PR (e9177a2) in Zashi. I can confirm

deprecated Send API still works ok
deprecated shileding API still works ok
new proposeTransfer API works ok
new proposeShielding API works ok
createProposedTransactions API works ok

str4d · 2024-03-07T18:04:02Z

Before we merge this, I'm going to add a helper method to Proposal to expose the number of steps, so that callers can anticipate how many events they will get from the async stream returned from createProposedTransactions.

nuttycom

cursory utACK, modulo @pacu's comments

nuttycom · 2024-03-08T00:34:49Z

Sources/ZcashLightClientKit/Synchronizer/SDKSynchronizer.swift

+        var iterator = transactions.makeIterator()
+        var submitFailed = false
+
+        return AsyncThrowingStream() {


I don't understand exactly how AsyncThrowingStream works - is it something whereby each time you poll it it invokes this block?

It's async stream and there's a very specific way how to work with it. The stream emits values until it finishes (nil value is emitted). And a client awaits it until the nil is emitted (or an error in case of AsyncThrowingStream - throwing means this stream can emit an error, another stream is AsyncStream which ends only when nil is emitted).

So technically speaking, typically it's not known how many values are emitted, it usually even doesn't matter because the use of stream is implemented as

for try await value in stream { }

This for will end at some point in the future, the stream can emit 1, 5 or 5M values. The processing lives in an async context so it's not blocking.

Sources/ZcashLightClientKit/Synchronizer/SDKSynchronizer.swift

nuttycom

re-utACK 72acbc6

str4d · 2024-03-08T14:49:54Z

Force-pushed to remove the 2.0.11 changelog bump, as @LukasKorba will make a few other release updates before the actual release commit.

str4d force-pushed the 1204-expose-proposals branch from 9afed2a to d4b0306 Compare February 21, 2024 01:14

str4d added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 21, 2024

str4d marked this pull request as ready for review February 21, 2024 01:15

str4d force-pushed the 1204-expose-proposals branch from 35cc57b to 93e9954 Compare February 21, 2024 01:18

str4d commented Feb 21, 2024

View reviewed changes

Sources/ZcashLightClientKit/Synchronizer/SDKSynchronizer.swift Outdated Show resolved Hide resolved

str4d marked this pull request as draft February 21, 2024 01:24

str4d force-pushed the 1204-expose-proposals branch from 93e9954 to 4d4d749 Compare February 22, 2024 01:09

str4d marked this pull request as ready for review February 22, 2024 01:09

nuttycom approved these changes Feb 22, 2024

View reviewed changes