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

refactor(AztecNr): Unify isEmpty/isZero and empty/zero API and Empty trait #4638

Closed
Tracked by #2783
benesjan opened this issue Feb 16, 2024 · 1 comment
Closed
Tracked by #2783
Assignees
Labels
C-aztec.nr Component: Aztec smart contract framework

Comments

@benesjan
Copy link
Contributor

We don't have consistent naming of functions which return an instance of an "empty"/"zero" object and the related functions (isEmpty etc.).

We diverged in a few places from empty because it became unclear. One of these cases was when we I changed the naming of AppendOnlyTreeSnapshot::empty to ::zero because there it was not clear whether it's a snapshot of a tree where all the nodes are zero or whether it represents a tree where the leaves are 0 but all the other levels are hashed.

I would use "zero" in cases where all the values are set to 0/are "falsey" and empty in the rest of the cases.

@github-project-automation github-project-automation bot moved this to Todo in A3 Feb 16, 2024
@LHerskind LHerskind changed the title Unify isEmpty/isZero and empty/zero API and Empty trait refactor(AztecNr): Unify isEmpty/isZero and empty/zero API and Empty trait Mar 8, 2024
@sklppy88 sklppy88 self-assigned this Mar 25, 2024
@sklppy88 sklppy88 added the C-aztec.nr Component: Aztec smart contract framework label Mar 25, 2024
@sklppy88
Copy link
Contributor

sklppy88 commented Mar 25, 2024

@benesjan:
A couple questions here.

  1. I see a trait for Empty, but not for Zero.
    Can / should I add a Zero trait?
  2. Why do some structures implement both an empty() and a zero() which return the exact same thing, I would like to prune this to only implement one at a time. Is it okay to nuke one of them, or is there a specific reason for having both ? (see EthAddress)

A heuristic I'm planning to use to tackle this is as follows:

If it is a data structure that holds primitives only -> is_zero
If it is a data structure that holds at least one non-primitives -> is_empty

Some examples of applications of the rule (current -> after):
NoteHeader (empty -> empty; same)
ReadRequestStatus (empty -> zero)
NullifierLeafPreimage (empty -> zero)
NullifierKeyValidationRequestContext (empty -> empty; same)
CallerContext (empty -> empty; same)
NullifierKeyValidationRequest (empty -> empty; same)
SideEffect (empty -> zero)
ReadRequest (empty -> zero)
SideEffectLinkedToNoteHash (empty -> zero)
L2ToL1Message (empty -> empty)
etc

This seems to make sense as a general rule, esp in the case of NullifierLeafPreimage, SideEffect etc as an "empty" side effect is not really clear, a zero one is a bit more accurate imo.

If anything it could make more sense to even move everything to zero just for consistency, even the things that currently do make sense as empty (the "containers" such as NoteHeader, CallerContext, etc). As zero is still generally accurate even for them.

wdyt ?

@sklppy88 sklppy88 moved this from Todo to In Progress in A3 Mar 25, 2024
@sklppy88 sklppy88 moved this from In Progress to In Review in A3 Mar 28, 2024
@github-project-automation github-project-automation bot moved this from In Review to Done in A3 Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-aztec.nr Component: Aztec smart contract framework
Projects
Archived in project
2 participants