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(dal): strong typing for TEE proof status #2733

Merged
merged 5 commits into from
Aug 29, 2024

Conversation

pbeza
Copy link
Collaborator

@pbeza pbeza commented Aug 26, 2024

What ❔

Introduce strong typing for the TEE proof generation status in the Rust code only (not in the database). This is a followup for:

This PR also aligns the status types with those implemented in proof_generation_dal.rs (specifically the unpicked status introduced in #2258).

Why ❔

Strong typing makes it easier to reason about the code and helps protect against subtle bugs.

Checklist

  • PR title corresponds to the body of PR (we generate changelog entries from PRs).
  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • Code has been formatted via zk fmt and zk lint.

@pbeza
Copy link
Collaborator Author

pbeza commented Aug 26, 2024

I'm not sure why the CI is erroring out on code I haven't even touched. 🤷‍♂️

error: the borrowed expression implements the required traits
  --> crates/zk_supervisor/src/commands/database/reset.rs:29:22
   |
29 |         logger::info(&msg_database_loading(MSG_DATABASE_RESET_GERUND, &dal.path));
   |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: change this to: `msg_database_loading(MSG_DATABASE_RESET_GERUND, &dal.path)`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrows_for_generic_args
   = note: `-D clippy::needless-borrows-for-generic-args` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(clippy::needless_borrows_for_generic_args)]`

error: the borrowed expression implements the required traits
 --> crates/zk_supervisor/src/commands/test/prover.rs:9:37
  |
9 |     let _dir_guard = shell.push_dir(&ecosystem.link_to_code.join("prover"));
  |                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: change this to: `ecosystem.link_to_code.join("prover")`
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrows_for_generic_args

@aon @popzxc @Deniallugo any idea?

@pbeza pbeza marked this pull request as ready for review August 26, 2024 12:52
@Deniallugo
Copy link
Contributor

I'm not sure why the CI is erroring out on code I haven't even touched. 🤷‍♂️

error: the borrowed expression implements the required traits
  --> crates/zk_supervisor/src/commands/database/reset.rs:29:22
   |
29 |         logger::info(&msg_database_loading(MSG_DATABASE_RESET_GERUND, &dal.path));
   |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: change this to: `msg_database_loading(MSG_DATABASE_RESET_GERUND, &dal.path)`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrows_for_generic_args
   = note: `-D clippy::needless-borrows-for-generic-args` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(clippy::needless_borrows_for_generic_args)]`

error: the borrowed expression implements the required traits
 --> crates/zk_supervisor/src/commands/test/prover.rs:9:37
  |
9 |     let _dir_guard = shell.push_dir(&ecosystem.link_to_code.join("prover"));
  |                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: change this to: `ecosystem.link_to_code.join("prover")`
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrows_for_generic_args

@aon @popzxc @Deniallugo any idea?

Sorry, it's an update of rust compiler i'll fix it, you could ignore

@popzxc
Copy link
Member

popzxc commented Aug 27, 2024

I've shared some thoughts on the subject elsewhere. In short, I would love to see what do we gain from this change (considering cons mentioned there) besides having strong typing as a fact.

@pbeza
Copy link
Collaborator Author

pbeza commented Aug 28, 2024

@popzxc, PTAL.

Re this: I reverted the changes related to adding the ENUM type for the status SQL column, but the rest of the code changes still seem relevant to me. Now, we only have enum TeeProofGenerationJobStatus in the Rust code:

#[derive(Debug, EnumString, Display)]
enum TeeProofGenerationJobStatus {
#[strum(serialize = "unpicked")]
Unpicked,
#[strum(serialize = "picked_by_prover")]
PickedByProver,
#[strum(serialize = "generated")]
Generated,
}

just like we have enum ProofGenerationJobStatus in proof_generation_dal.rs:

#[derive(Debug, EnumString, Display)]
enum ProofGenerationJobStatus {
#[strum(serialize = "unpicked")]
Unpicked,
#[strum(serialize = "picked_by_prover")]
PickedByProver,
#[strum(serialize = "generated")]
Generated,
#[strum(serialize = "skipped")]
Skipped,
}

I also aligned with the unpicked status introduced by @Artemka374 in #2258. Not sure why he didn’t convert ready_to_be_proven to unpicked in the migration script, though (like I did). 🤔

@popzxc popzxc requested a review from perekopskiy August 29, 2024 07:00
@popzxc
Copy link
Member

popzxc commented Aug 29, 2024

@perekopskiy could you PTAL?

@perekopskiy perekopskiy changed the title chore(dal): strong typing for TEE proof status refactor(dal): strong typing for TEE proof status Aug 29, 2024
@perekopskiy
Copy link
Contributor

nit: it's not chore but rather refactor

@haraldh haraldh added this pull request to the merge queue Aug 29, 2024
Merged via the queue into main with commit e1e721e Aug 29, 2024
49 checks passed
@haraldh haraldh deleted the tee/db-status-strong-typing branch August 29, 2024 10:10
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.

5 participants