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

Modules: WorkingSet refactoring. #384

Merged
merged 11 commits into from
Jun 5, 2023
Merged

Modules: WorkingSet refactoring. #384

merged 11 commits into from
Jun 5, 2023

Conversation

bkolad
Copy link
Member

@bkolad bkolad commented Jun 5, 2023

Description

This PR splits WorkingSet enum into two types WorkingSet & CommittedWorkingSet. This has the following advantages:

  1. The AppTemplate now stores CommittedWorkingSet, ensuring that only the state that has been committed in the apply_batch method is stored in the database during the end_slot phase

  2. The commit & revert methods are exclusive to the WorkingSet type and return a CommittedWorkingSet instance. Additionally, only the CommittedWorkingSet type has to_revertable() method. This explicit state transition between WorkingSet and CommittedWorkingSet prevents the accidental reversion of a CommittedWorkingSet or the storage of a WorkingSet in the database without committing (or reverting) it beforehand.

Linked Issues

Testing

All relevant tests ware updated.

Docs

Updated docs for WorkingSet & CommittedWorkingSet

@codecov
Copy link

codecov bot commented Jun 5, 2023

Codecov Report

Merging #384 (9cdffd0) into main (1fdae7a) will decrease coverage by 0.1%.
The diff coverage is 97.6%.

❗ Current head 9cdffd0 differs from pull request most recent head 1ed7585. Consider uploading reports for the commit 1ed7585 to get more accurate results

Impacted Files Coverage Δ
module-system/sov-modules-stf-template/src/lib.rs 82.9% <81.8%> (ø)
...odule-implementations/module-template/src/tests.rs 100.0% <100.0%> (ø)
module-system/sov-state/src/prover_storage.rs 95.9% <100.0%> (+<0.1%) ⬆️
module-system/sov-state/src/scratchpad.rs 95.1% <100.0%> (+3.8%) ⬆️
module-system/sov-state/src/state_tests.rs 100.0% <100.0%> (ø)

... and 7 files with indirect coverage changes

@bkolad bkolad changed the title Modules: Working set refactoring. Modules: WorkingSet refactoring. Jun 5, 2023
Copy link
Member

@preston-evans98 preston-evans98 left a comment

Choose a reason for hiding this comment

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

This change looks great overall, but I'm wondering if we can rename some things to clarify roles. (This isn't blocking, since the problems pre-date this PR).

Currently, the workflow to commit some changes to the DB is storage.validate_and_commit(working_set.commit().freeze()), which seems really cumbersome.

Maybe we can rename CommittedWorkingSet to StateCheckpoint or something. And then commit() could get renamed to checkpoint(). As for freeze - maybe we could rename to split_for_storage() or something?

If anyone has suggestions for renaming any of these items, leave them below! cc @citizen-stig @bkolad @dubbelosix

@bkolad bkolad mentioned this pull request Jun 5, 2023
@bkolad
Copy link
Member Author

bkolad commented Jun 5, 2023

This change looks great overall, but I'm wondering if we can rename some things to clarify roles. (This isn't blocking, since the problems pre-date this PR).

Currently, the workflow to commit some changes to the DB is storage.validate_and_commit(working_set.commit().freeze()), which seems really cumbersome.

Maybe we can rename CommittedWorkingSet to StateCheckpoint or something. And then commit() could get renamed to checkpoint(). As for freeze - maybe we could rename to split_for_storage() or something?

If anyone has suggestions for renaming any of these items, leave them below! cc @citizen-stig @bkolad @dubbelosix

Added an issue: #388

@bkolad bkolad merged commit a0f6ff4 into main Jun 5, 2023
@bkolad bkolad deleted the blaze/working_set_refactor branch June 5, 2023 18:51
@bkolad bkolad restored the blaze/working_set_refactor branch June 6, 2023 08:53
@bkolad bkolad deleted the blaze/working_set_refactor branch June 6, 2023 12:48
@citizen-stig citizen-stig linked an issue Jun 6, 2023 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.

Better naming in sov-state
3 participants