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(StateVariables): Migrate Storage<T> to Storage<N> #5736

Open
Tracked by #2783
nventuro opened this issue Apr 12, 2024 · 0 comments · May be fixed by #10320
Open
Tracked by #2783

refactor(StateVariables): Migrate Storage<T> to Storage<N> #5736

nventuro opened this issue Apr 12, 2024 · 0 comments · May be fixed by #10320

Comments

@nventuro
Copy link
Contributor

nventuro commented Apr 12, 2024

We currently allocate storage slots using the Storage trait, which reads:

trait Storage<T> where T: Serialize<N> + Deserialize<N> {
    fn get_storage_slot(self) -> Field {
        self.storage_slot
    }
}

Note that we don't use the serde traits anywhere, nor the N value. However, the macros do rely on N to determine how many slots to allocate. This implies that a state variable that holds some type T will require exactly as many slots as it'd need to store a serialized version of T. However, there's many scenarios in which this is not true.

SharedMutable requires 2 * N + 1 slots (a ScheduledValueChange struct, with pre post and block_of_change). A dynamic array will likely require a single slot to store the length, regardless of T. Map similarly also uses a single slot.

I propose we change the storage definition to:

trait Storage<N> {
    fn get_storage_slot(self) -> Field {
        self.storage_slot
    }
}

and use this N value directly in the macro to determine how many slots to allocate. We could then have things like:

impl<K, T> Storage<1> for Map<K, T> {}

impl<T> Storage<N> for PublicMutable<T> where T: Serialize<N> + Deserialize<N> {}

impl<T> Storage<2 * N + 1> for SharedMutable<T> where T: Serialize<N> + Deserialize<N> {}

Note that we won't be able to do arithmetic until noir-lang/noir#4784 is implemented, but we could already do the rest.

@github-project-automation github-project-automation bot moved this to Todo in A3 Apr 12, 2024
@LHerskind LHerskind changed the title Migrate Storage<T> to Storage<N> refactor(StateVariables): Migrate Storage<T> to Storage<N> Apr 15, 2024
nventuro added a commit that referenced this issue Apr 22, 2024
Closes #4760

This fully migrates the `TokenBlacklist` contract to use a
`SharedMutable` state variable instead of the slow updates tree.
It mostly consists of deleting things we no longer need, like tree
simulation, initialization, capsule creation and pushing, etc.

The only relevant change is that `SharedMutable` does not allow for
scheduling from private. This is trivially achievable though by making
an internal public function that does this and then privately call it
(which is what the slow tree did), but this hardly seemed relevant so I
simply made the `set_roles` function public.

---

## Macros and Storage Slots

This PR also references
#5736, since it
shows how that forces us to implement `Serialize` even when we don't
need to. It also ends up being useless, since what we need to store is a
`ScheduledValueChange<UserFlags>`, which has a serialize length of 3,
and not a `UserFlags` (even though the state variable is
`SharedMutable<UserFlags>`). It's a good example of state variables
implementing custom data structures that may require storage separate
from the underlying type.

---------

Co-authored-by: ludamad <[email protected]>
@nventuro nventuro assigned nventuro and unassigned nventuro May 13, 2024
@sklppy88 sklppy88 self-assigned this Aug 6, 2024
@sklppy88 sklppy88 removed their assignment Aug 21, 2024
@nventuro nventuro linked a pull request Nov 29, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Todo
Development

Successfully merging a pull request may close this issue.

2 participants