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

Add Deref to newtypes, add casts to double newtypes #170

Merged
merged 1 commit into from
Jan 17, 2024

Conversation

giladchase
Copy link
Contributor

@giladchase giladchase commented Jan 16, 2024

  • QOL
  • Nobody should have to handle PatriciaKey directly, it's an implementation detail.

This change is Reviewable

@giladchase giladchase changed the title TAdd Deref to newtypes, add casts to double newtypes Add Deref to newtypes, add casts to double newtypes Jan 16, 2024
- QOL
- Nobody should have to handle `PatriciaKey` directly, it's an
implementation detail.
Copy link
Contributor Author

@giladchase giladchase left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @elintul and @yair-starkware)


src/core.rs line 55 at r1 (raw file):

    fn from(contract_address: ContractAddress) -> StarkFelt {
        **contract_address
    }

While pretty IMO, this might be too implicity, see below.

This works due to *x being rust sugar for *(x.deref()), and since rust adds a & implicitly to self if regular method resolution fails, so we have:

// below `==>` means "gets compiled into"
**contract_address ==> 
**(contract_address.deref()) => 
*(*(contract_address.deref()).deref()) =>
*(*((&contract_address).deref()).deref()) =>
*(*(&patricia_key).deref()) =>
*(patricia_key.deref()) =>
*((&patricia_key).deref()) =>
*(&stark_felt) =>
stark_felt

Code quote:

    fn from(contract_address: ContractAddress) -> StarkFelt {
        **contract_address
    }

Copy link
Contributor Author

@giladchase giladchase left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware, @elintul, and @yair-starkware)


src/core.rs line 55 at r1 (raw file):

Previously, giladchase wrote…

While pretty IMO, this might be too implicity, see below.

This works due to *x being rust sugar for *(x.deref()), and since rust adds a & implicitly to self if regular method resolution fails, so we have:

// below `==>` means "gets compiled into"
**contract_address ==> 
**(contract_address.deref()) => 
*(*(contract_address.deref()).deref()) =>
*(*((&contract_address).deref()).deref()) =>
*(*(&patricia_key).deref()) =>
*(patricia_key.deref()) =>
*((&patricia_key).deref()) =>
*(&stark_felt) =>
stark_felt

The explicit version will be *contract_address.0.key()

@giladchase giladchase requested review from elintul and removed request for elintul January 16, 2024 13:49
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @elintul, @giladchase, and @yair-starkware)

a discussion (no related file):
what is (are) the use case(s)?



src/core.rs line 55 at r1 (raw file):

Previously, giladchase wrote…

The explicit version will be *contract_address.0.key()

is there a runtime difference, do you think? I would go with speed if so

Copy link
Contributor Author

@giladchase giladchase left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware, @elintul, and @yair-starkware)

a discussion (no related file):

Previously, dorimedini-starkware wrote…

what is (are) the use case(s)?

Most our usage of .0 in the blockifier originates from these types.
Other than being boilerplate, using .0 is a bit vague, because users can't tell at first glance if this is a newtype or a large tuple.

For example, this allows one to use StarkFelt methods directly on Nonce, and ClassHash instances, or pass (references of) them into functions that expect a StarkFelt, without having to .0 first.

For types like ContractAddress this is doubly true, because users have to go through an implementation detail, PatriciaKey, in order to get to the inner starkFelt .
It ends up being that any operation one wants to do on the value of the contract_address requires doing *contract_address.0.key(), whereas now it will be contract_address,into().



src/core.rs line 55 at r1 (raw file):

Previously, dorimedini-starkware wrote…

is there a runtime difference, do you think? I would go with speed if so

The ** is probably either faster or both solutions are optimized into the same thing.

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @elintul and @yair-starkware)

@giladchase giladchase added this pull request to the merge queue Jan 17, 2024
Merged via the queue into main with commit 25f106b Jan 17, 2024
6 checks passed
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.

2 participants