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

Improve store and state sync #68

Merged
merged 12 commits into from
Dec 29, 2023
Merged

Improve store and state sync #68

merged 12 commits into from
Dec 29, 2023

Conversation

igamigo
Copy link
Collaborator

@igamigo igamigo commented Dec 21, 2023

  • Modifies the store to enable the possibility to store RecordedNote as well as Notes, in order to be able to track notes after transaction execution and update the notes with their respective inclusion proofs on state sync
    • This is a bit messy but for now it was the simplest way to implement this
  • Also changes it so that the AccountStorage is stored in place of the slots in order to be able to reconstruct Account for the DataStore

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left some comments inline.

src/cli/account.rs Outdated Show resolved Hide resolved
src/store/state_sync.rs Outdated Show resolved Hide resolved
src/store/state_sync.rs Show resolved Hide resolved
src/store/notes.rs Outdated Show resolved Hide resolved
src/client/transactions.rs Outdated Show resolved Hide resolved
src/client/sync_state.rs Outdated Show resolved Hide resolved
src/cli/transactions.rs Outdated Show resolved Hide resolved
src/client/notes.rs Outdated Show resolved Hide resolved
@igamigo igamigo marked this pull request as ready for review December 26, 2023 20:13
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left a few comments inline - but they are mostly nits. Once these are addressed, we can merge.

src/store/notes.rs Outdated Show resolved Hide resolved
src/store/notes.rs Outdated Show resolved Hide resolved
src/store/state_sync.rs Outdated Show resolved Hide resolved
src/cli/input_notes.rs Show resolved Hide resolved
Comment on lines 105 to 109
fn get_newly_committed_note_hashes(
&self,
notes: &[NoteSyncRecord],
block_header: &BlockHeader,
) -> Result<Vec<(Digest, NoteInclusionProof)>, ClientError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the name of the function can probably be improved since we are not really getting note hashes here. Maybe something like get_newly_committed_note_info.

Also, would be great to add some brief doc comments to this function to explain what it does.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, changed

src/store/state_sync.rs Outdated Show resolved Hide resolved
src/client/transactions.rs Outdated Show resolved Hide resolved
src/client/transactions.rs Outdated Show resolved Hide resolved
src/client/transactions.rs Outdated Show resolved Hide resolved
@igamigo igamigo merged commit 1238769 into main Dec 29, 2023
4 checks passed
@igamigo igamigo deleted the igamigo-impl-data-store branch December 29, 2023 20:13
@igamigo igamigo linked an issue Jan 4, 2024 that may be closed by this pull request
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.

Enable importing/exporting for both RecordedNote and Note
2 participants