-
Notifications
You must be signed in to change notification settings - Fork 586
Archive: migrate non-nullable zkapp state as well #17827
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
Archive: migrate non-nullable zkapp state as well #17827
Conversation
| RAISE DEBUG 'Adding column % for zkapp_states', col_name; | ||
|
|
||
| EXECUTE format( | ||
| 'ALTER TABLE zkapp_states ADD COLUMN IF NOT EXISTS %I INT DEFAULT 0 NOT NULL REFERENCES zkapp_field(id)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you reason why 0 not null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MIP explicitly specify we should use 0 as polyfill.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing that zkapp_states_nullable seems to be used for is constructing account updates:
mina/src/app/archive/lib/load_data.ml
Lines 512 to 527 in 90ff48c
| let%bind state = | |
| let%bind elements = | |
| query_db ~f:(fun db -> | |
| Processor.Zkapp_states_nullable.load db state_id ) | |
| in | |
| let elements = Pickles_types.Vector.to_list elements in | |
| let%map fields = | |
| Deferred.List.map elements ~f:(fun id_opt -> | |
| Option.value_map id_opt ~default:(return None) ~f:(fun id -> | |
| let%map field_str = | |
| query_db ~f:(fun db -> Processor.Zkapp_field.load db id) | |
| in | |
| Some (Zkapp_basic.F.of_string field_str) ) ) | |
| in | |
| List.map fields ~f:Or_ignore.of_option |> Zkapp_state.V.of_list_exn | |
| in |
That state contains a vector of F.t Or_ignore.t - the field elements are allowed to be absent, I think because the update might only affect the state at certain indices.
Should we be adding 0 as a value to the update in that case? If this is only used for updates then null seems more correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ I assume this comment is for PR #17826
e1ad22b to
b747663
Compare
3964e50 to
ea19fa6
Compare
ea19fa6 to
e655d54
Compare
b747663 to
d243526
Compare
94d0061 to
a7988ad
Compare
This PR added migration for non-nullable zkapp state table.
TODO:
previous: #17826