Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Type safe address api #491

Merged
merged 4 commits into from
Sep 7, 2022

Conversation

pacu
Copy link
Contributor

@pacu pacu commented Aug 21, 2022

Closes #461
Closes #510
Closes #517
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.

@pacu pacu marked this pull request as draft August 21, 2022 16:21
@nuttycom nuttycom force-pushed the feature/zip-316-and-latest-upstream branch from e348740 to 905ee40 Compare August 24, 2022 15:42
@pacu pacu force-pushed the type_safe_address_api branch from 6290dcf to ed2dc40 Compare August 26, 2022 22:09
@pacu pacu requested a review from ccjernigan August 27, 2022 00:19
@pacu
Copy link
Contributor Author

pacu commented Aug 27, 2022

@str4d @nuttycom I updated the code to this branch that builds librustzcash_0_7

https://github.com/zcash-hackworks/zcash-light-client-ffi/tree/bin/librustzcash_0_7

Some of the tests to derive keys fail.

  Assertions: Assertion Failure at DerivationToolTestnetTests.swift:99: XCTAssertEqual failed: ("UnifiedFullViewingKey(encoding: "uviewtest12tkgzhaevmw78us4xj2cx6ehxjgpp5da2qwrjqvytztejqfjdmy3e6nryqggtwrjum5cefuuuky8rscuw5dynmjec2tx3kkupqexw4va879pf874kvp6r8kjeza26gysxllaqwl67hm9u0jjke06zc93asrpw4wmy3g0lr9r5cy9pz49q2g7y7wm2pls5akmzhuvqr7khftk93aa2kpvwp7n3sjtmef28mxg3n2rpctsjlgsrhc29g6r23qc0u4tzd8rz8vqq4j7jxummdts8zx0jatzw4l2tl7r3egxhlw587rtkjx0y6dvw4hf4vjprn0qv3hs0sulmavk84ajeewn7argyerpr4essqvgfd0d24jpz6phxlasnd58qazh9d3yc6ad3hc5atp0pkvlq053zga65gscp0pv2plhqj9y2tcmx43thw5g4v8z3unytkc2dhyttuhmnlh5dyz4rmhgfkc96tp8z8rpfe35whjvky0jagz5n7qx", account: 0)") is not equal to 
("UnifiedFullViewingKey(encoding: "uviewtest16dqd5q7zd3xfxlcm2jm5k95zd92ed3qcm5jr9uq6yl5y2h6vuwfpxlnndckv5hpwsajgvq26xgdcdns8mqclecl0zh4sph45t4ncfnhcsus0k6sashfknsp9ltxrxlf096ljkwmp7psh3z2vmcd3rcc72qaujsl2y23ajexhr7qza29u9k95frs8qqgvy83rgymt7mmw09a02a5ytjpa502tshlsgl2j736jlfuzt27gezlajrs2tw9c99uqxrj5sx942vdr7w6yk2ltz96yq7n96fd9nr48c59dh9znqtwtm0nt9tmt7vzwhdwzt00tgp57mn85hpe6w00upmjv52kct9y", account: 0)")
  File: DerivationToolTestnetTests.swift:9

Also it appears that key validation functions don't produce errors when the key is semantically correct but belongs to a different network

func testSpendingKeyValidationThrowsWhenWrongNetwork() throws {
        XCTAssertThrowsError(try derivationTool.isValidExtendedSpendingKey("secret-extended-key-main1qw28psv0qqqqpqr2ru0kss5equx6h0xjsuk5299xrsgdqnhe0cknkl8uqff34prwkyuegyhh5d4rdr8025nl7e0hm8r2txx3fuea5mquy3wnsr9tlajsg4wwvw0xcfk8357k4h850rgj72kt4rx3fjdz99zs9f4neda35cq8tn3848yyvlg4w38gx75cyv9jdpve77x9eq6rtl6d9qyh8det4edevlnc70tg5kse670x50764gzhy60dta0yv3wsd4fsuaz686lgszc7nc9vv"))
    }
  Assertions: Assertion Failure at DerivationToolTestnetTests.swift:113: XCTAssertThrowsError failed: did not throw an error
  File: DerivationToolTestnetTests.swift:113

Copy link
Contributor

@nuttycom nuttycom left a comment

Choose a reason for hiding this comment

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

Flushing comments.

Package.resolved Outdated Show resolved Hide resolved
Package.swift Outdated Show resolved Hide resolved
Sources/ZcashLightClientKit/Model/WalletTypes.swift Outdated Show resolved Hide resolved
Sources/ZcashLightClientKit/Rust/ZcashRustBackend.swift Outdated Show resolved Hide resolved
Copy link
Contributor

@nuttycom nuttycom left a comment

Choose a reason for hiding this comment

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

utACK for the current unit of work to achieve an intermediate state that allows internal testing; however, I think that we should essentially change everything over to work in terms of unified keys before we publish a new release.

Copy link
Collaborator

@LukasKorba LukasKorba left a comment

Choose a reason for hiding this comment

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

Added only a few non-blocking comments, otherwise LGTM

This PR creates data types for Addresses and Keys so that they are
not represented by Strings anymore. This avoids mistakenly use
the wrong keys because they are all alike for the type system.

New Protocols:
=============

StringEncoded -> Protocol that makes a type can be expressed in an
string-encoded fashion either for UI or Interchange purposes.

Undescribable -> A protocol that implements methods that override default
decriptions used by debuggers, loggers and event trackers to avoid types
conforming to it to be leaked to logs.

Deleted Protocols:
==================

UnifiedFullViewingKey --> turned into a struct.
UnifiedAddress --> turned into a struct

new Error Type:
================

````
enum KeyEncodingError: Error {
    case invalidEncoding
}
````

This error is thrown when an Address or Key type (addresses are public
keys in the end) can be decoded from their String representation,
typically upon initialization from a User input.

New Types:
=========

SaplingExtendedSpendingKey -> Type for Sapling Extended Full Viewing Keys
this type will be replaced with Unified Spending Keys soon.

SaplingExtendedFullViewingKey -> Extended Full Viewing Key for Sapling.
Maintains existing funcionality. Will be probably deprecated in favor of
UFVK.

TransparentAccountPrivKey -> Private key for transparent account. Used
only for shielding operations. Note: this will probably be deprecated soon.

UnifiedFullViewingKey -> Replaces the protocol that had the same name.

TransparentAddress -> Replaces a type alias with a struct

SaplingAddress --> Represents a Sapling receiver address. Comonly called zAddress. This address corresponds to the Zcash Sapling shielded pool.
Although this it is fully functional, we encourage developers to
choose `UnifiedAddress` before Sapling or Transparent ones.

UnifiedAddress -> Represents a UA. String-encodable and Equatable. Use of
UAs must be favored instead of individual receivers for different pools.
This type can't be decomposed into their Receiver types yet.

Recipient -> This represents all valid receiver types to be used as
inputs for outgoing transactions.

````
public enum Recipient: Equatable, StringEncoded {
    case transparent(TransparentAddress)
    case sapling(SaplingAddress)
    case unified(UnifiedAddress)
````

The wrapped concrete receiver is a valid receiver type.

Deleted Type Aliases:
=====================

The following aliases were deleted and turned into types
````
public typealias TransparentAddress = String
public typealias SaplingShieldedAddress = String

````

Changes to Derivation Tool
==========================

DerivationTool has been changed to accomodate this new types and
remove Strings whenever possible.

Changes to Synchronizer and CompactBlockProcessor
=================================================
Accordingly these to components have been modified to accept the
new types intead of strings when possible.

Changes to Demo App
===================
The demo App has been patch to compile and work with the new types.
Developers must consider that the use (and abuse) of forced_try and
forced unwrapping is a "license" that maintainers are using for the
sake of brevity. We consider that clients of this SDK do know how to
handle Errors and Optional and it is not the objective of the demo
code to show good practices on those matters.

Closes #461
@pacu pacu force-pushed the type_safe_address_api branch from 666b287 to ed87a27 Compare September 5, 2022 18:09
Copy link
Contributor

@nuttycom nuttycom left a comment

Choose a reason for hiding this comment

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

This is looking good; there are just a few more things that need to be addressed.

@pacu pacu requested a review from nuttycom September 6, 2022 18:26
Copy link
Contributor

@nuttycom nuttycom left a comment

Choose a reason for hiding this comment

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

utACK; additional required changes are tracked as #521 and #522

@nuttycom nuttycom merged commit a8e6b5b into feature/zip-316-and-latest-upstream Sep 7, 2022
@nuttycom nuttycom deleted the type_safe_address_api branch September 7, 2022 15:20
@pacu pacu mentioned this pull request Sep 19, 2022
@nuttycom nuttycom mentioned this pull request Oct 6, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants