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(accounts): Store signing public keys in immutable private notes in account contracts #1080

Merged
merged 10 commits into from
Jul 19, 2023

Conversation

spalladino
Copy link
Collaborator

@spalladino spalladino commented Jul 14, 2023

  • Properly stores and loads the public key from an ImmutableSingleton in an ecdsa account contract.
  • More fixes to e2e account test.
  • Add new debug call in Noir to log an array with a string prefix (so we know what we're logging since they print out of order)
  • Rollback Using bb ECDSA #954 since barretenberg ECDSA doesn't verify in Noir.
  • Reenable tests for ECDSA account contract.
  • Rename current Schnorr account to SchnorrSingleKey.
  • Introduce new SchnorrMultiKey account contract that stores its signing key as a private note following the same approach as the refactored ECDSA account contract.

Fixes #1007

@spalladino spalladino force-pushed the palla/handle-ecdsa-note branch from 0c6730f to 787b796 Compare July 14, 2023 22:25
@spalladino spalladino changed the title fix: load signing public key from immutable singleton note in ecdsa account contract Reenable ECDSA account contract Jul 14, 2023
@spalladino spalladino requested a review from PhilWindle July 14, 2023 22:51
Comment on lines +81 to +87
context = emit_encrypted_log(
context,
this,
storage.public_key.storage_slot,
get_public_key(this),
pub_key_note.serialise(),
);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@LeilaWang should this be part of ImmutableSingleton#initialise? Or is there a scenario where we may want to initialise it without emitting the corresponding encrypted log?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually thought about the same - emitting a log when a note is created (in singleton.initialise, set.insert, etc). But then decided to keep it separate. Because in some use cases we might want to emit unencrypted logs instead of encrypted ones. And in some use cases, like you said, we might not want to emit anything at all (for example, the ClaimNote in zk_token_contract).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've added a header to a note in a recent PR, and will change the emit apis to be cleaner since we won't be passing context, contract address and storage slot around :)

@spalladino spalladino marked this pull request as ready for review July 14, 2023 22:53
@spalladino spalladino changed the title Reenable ECDSA account contract feat: Reenable ECDSA account contract Jul 17, 2023
@spalladino spalladino changed the title feat: Reenable ECDSA account contract feat(accounts): Reenable ECDSA account contract Jul 17, 2023
@spalladino spalladino requested a review from LeilaWang July 18, 2023 12:17
@spalladino spalladino force-pushed the palla/handle-ecdsa-note branch from 478a22d to 4028f5f Compare July 18, 2023 16:46
@spalladino spalladino changed the title feat(accounts): Reenable ECDSA account contract feat(accounts): Store signing public keys in immutable private notes in account contracts Jul 18, 2023
@spalladino spalladino enabled auto-merge (squash) July 18, 2023 17:51
@spalladino spalladino disabled auto-merge July 19, 2023 12:59
Copy link
Contributor

@benesjan benesjan left a comment

Choose a reason for hiding this comment

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

Just minor issues. Happy to approve once addressed.

Comment on lines +81 to +87
context = emit_encrypted_log(
context,
this,
storage.public_key.storage_slot,
get_public_key(this),
pub_key_note.serialise(),
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually thought about the same - emitting a log when a note is created (in singleton.initialise, set.insert, etc). But then decided to keep it separate. Because in some use cases we might want to emit unencrypted logs instead of encrypted ones. And in some use cases, like you said, we might not want to emit anything at all (for example, the ClaimNote in zk_token_contract).

Comment on lines +81 to +87
context = emit_encrypted_log(
context,
this,
storage.public_key.storage_slot,
get_public_key(this),
pub_key_note.serialise(),
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've added a header to a note in a recent PR, and will change the emit apis to be cleaner since we won't be passing context, contract address and storage slot around :)

@spalladino spalladino enabled auto-merge (squash) July 19, 2023 14:07
@spalladino spalladino merged commit 3201321 into master Jul 19, 2023
@spalladino spalladino deleted the palla/handle-ecdsa-note branch July 19, 2023 14:17
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.

Modify schnorr and ecdsa account contracts to store the public key
3 participants