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

Rename sk_r and pk_r to note_.. #157

Merged
merged 1 commit into from
Apr 22, 2024
Merged

Rename sk_r and pk_r to note_.. #157

merged 1 commit into from
Apr 22, 2024

Conversation

moCello
Copy link
Member

@moCello moCello commented Apr 19, 2024

Resolves #156

@moCello moCello force-pushed the mocello/156_note_keys branch from c0b5dc9 to d432b25 Compare April 19, 2024 17:02
Copy link
Member

@xevisalle xevisalle left a comment

Choose a reason for hiding this comment

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

Just the comment about the address.

Plus, as a reflexion, I'd prefer the variables to be called nsk and npk, but I understand that calling em note_sk and note_pk is much more readable, so it's fine to me!

}

/// Gets the underline `JubJubExtended` point of `pk_r`
/// Gets the underline `JubJubExtended` point of `note_pk`
pub fn address(&self) -> &JubJubExtended {
Copy link
Member

Choose a reason for hiding this comment

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

as this is returning the note_pk, I'd call this function like this. In our context the address is the static public key. Otherwise if we refer to the stealth address, we must consider the tuple (note_pk, R)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm also not entirely happy with the address function.

We already have a method called note_pk (formerly pk_r) of the StealthAddress that returns a NotePublicKey (which is just a wrapped JubJubExtended) so I cannot use the same function name here again.

If we want to get the internal point in NotePublicKey we currently have two options:

// without `address` method
let jubjub_point = stealth_address.note_pk().as_ref();

// with `address` method
let jubjub_point = stealth_address.address();

I think the second one is nicer but of course only if the concept of an 'address' somewhat makes sense.

Any thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Adding my two cents here. I think having a StealthAddress::address function is a bit... redundant to say the least, and confusing in the worst case. That said, I also understand the stealth_address.note_pk().as_ref() is unsatisfactory.

Maybe we should improve the access to the underlying JubJubScalar in NotePublicKey in a future PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree and removed the address method. I also agree that we need to address the as_ref() API, this needs to be done in jubjub-schnorr though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@HDauven HDauven left a comment

Choose a reason for hiding this comment

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

LGTM

Added two places where the docs might be updated. Will approve and leave it up to you whether to change it or not.

src/keys/public.rs Outdated Show resolved Hide resolved
src/keys/view.rs Outdated Show resolved Hide resolved
@moCello
Copy link
Member Author

moCello commented Apr 22, 2024

Plus, as a reflexion, I'd prefer the variables to be called nsk and npk, but I understand that calling em note_sk and note_pk is much more readable, so it's fine to me!

Yes, that's also an option. I'll do it like this in the changes and the stack bump because if I follow the convention npk for NotePublicKey, then I'll need to name the StakeSecretKey in the stake contract ssk which will be very confusing with the formerly known SecretSpendKey. To avoid such confusion I'll stick to the longer names for now.
In the long run I think we'll see both options in our crates.

Copy link
Member

@ureeves ureeves left a comment

Choose a reason for hiding this comment

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

Very nice. Approved. Added my two cents to the address debate

}

/// Gets the underline `JubJubExtended` point of `pk_r`
/// Gets the underline `JubJubExtended` point of `note_pk`
pub fn address(&self) -> &JubJubExtended {
Copy link
Member

Choose a reason for hiding this comment

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

Adding my two cents here. I think having a StealthAddress::address function is a bit... redundant to say the least, and confusing in the worst case. That said, I also understand the stealth_address.note_pk().as_ref() is unsatisfactory.

Maybe we should improve the access to the underlying JubJubScalar in NotePublicKey in a future PR?

@moCello moCello force-pushed the mocello/156_note_keys branch from 2994001 to 560b72e Compare April 22, 2024 11:49
Also remove `StealthAddress::address` method since it is a duplicate of
`StealthAddress.note_pk().as_ref()`.

Resolves #156
Copy link
Member

@xevisalle xevisalle left a comment

Choose a reason for hiding this comment

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

LGTM

@moCello moCello merged commit ea9e863 into master Apr 22, 2024
5 checks passed
@moCello moCello deleted the mocello/156_note_keys branch April 22, 2024 14:15
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.

Rename SecretKey::sk_r to generate_note_sk and StealthAddress::pk_r to note_pk
4 participants