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

feat: CompleteAddress type #1524

Merged
merged 38 commits into from
Aug 15, 2023
Merged

feat: CompleteAddress type #1524

merged 38 commits into from
Aug 15, 2023

Conversation

benesjan
Copy link
Contributor

@benesjan benesjan commented Aug 11, 2023

  1. Fixes Introduce a new type wrapping PartialAddress, PublicKey and AztecAddress #1516
  2. Fixes Expose addAccount api as Aztec CLI command #1527
  3. Fixes Rename 'registerPublicKey' to 'registerRecipient' #1529
  4. Documents e2e_aztec_js_browser.
  5. Fixes unexpected "previous test run dependency" in e2e_aztec.js_browser test.

Note: When implementing this PR I've stumbled upon this issue. I would wait for @spypsy to come back before dealing with this since he's the most knowledgeable about that part of codebase.

Checklist:

Remove the checklist to signal you've completed it. Enable auto-merge if the PR is ready to merge.

  • If the pull request requires a cryptography review (e.g. cryptographic algorithm implementations) I have added the 'crypto' tag.
  • I have reviewed my diff in github, line by line and removed unexpected formatting changes, testing logs, or commented-out code.
  • Every change is related to the PR description.
  • I have linked this pull request to relevant issues (if any exist).

@benesjan benesjan marked this pull request as draft August 11, 2023 11:19
@benesjan benesjan force-pushed the janb/complete-address-type branch 3 times, most recently from 820c72b to a39a74e Compare August 11, 2023 14:57
@benesjan benesjan marked this pull request as ready for review August 11, 2023 15:32
PhilWindle
PhilWindle previously approved these changes Aug 11, 2023
@PhilWindle PhilWindle self-requested a review August 12, 2023 07:25
@PhilWindle PhilWindle dismissed their stale review August 12, 2023 07:26

Needs further work

Copy link
Collaborator

@PhilWindle PhilWindle left a comment

Choose a reason for hiding this comment

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

The docs/example code for sandbox and aztec cli will need to be updated.

@benesjan benesjan force-pushed the janb/complete-address-type branch 2 times, most recently from 7e91992 to 6a9cf25 Compare August 14, 2023 09:48
@benesjan benesjan force-pushed the janb/complete-address-type branch from 6a9cf25 to 9577cc7 Compare August 14, 2023 10:14
@benesjan benesjan marked this pull request as draft August 14, 2023 14:59
@benesjan benesjan force-pushed the janb/complete-address-type branch from 2f90170 to d15bfcf Compare August 15, 2023 10:43
@benesjan benesjan marked this pull request as ready for review August 15, 2023 11:13
@benesjan benesjan requested a review from spalladino August 15, 2023 11:13
Copy link
Collaborator

@spalladino spalladino left a comment

Choose a reason for hiding this comment

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

Wow, this was a massive change, thanks for taking it on @benesjan! And yeah, the resulting code is a lot cleaner.

Comment on lines +86 to +89
// TODO: Re-enable this check once https://github.com/AztecProtocol/aztec-packages/issues/1556 is solved
// if (!pubKey.equals(account.publicKey)) {
// throw new Error(`Public key mismatch: ${pubKey.toString()} != ${account.publicKey.toString()}`);
// }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that this was re-enabled on master

@benesjan benesjan dismissed PhilWindle’s stale review August 15, 2023 12:31

Docs were already updated and Phil is on vacations so I need to dismiss this to be able to merge the PR.

@benesjan benesjan enabled auto-merge (squash) August 15, 2023 13:02
@benesjan benesjan merged commit aa2c74c into master Aug 15, 2023
@benesjan benesjan deleted the janb/complete-address-type branch August 15, 2023 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
3 participants