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

feat: switch to sqlite instead of json for state persistence #117

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

neriumrevolta
Copy link
Contributor

@neriumrevolta neriumrevolta commented Nov 9, 2023

Description

  • PersistedState struct removed in favour of passing a db pool reference (&SqlitePool) wherever we need to update/query the db
  • updated unit tests (they were on the state file before)
  • updated e2e tests

@neriumrevolta neriumrevolta force-pushed the petko/migrate-state-from-json-to-db branch 9 times, most recently from 7da6287 to 4ebfe76 Compare November 16, 2023 14:18
@neriumrevolta neriumrevolta force-pushed the petko/migrate-state-from-json-to-db branch 7 times, most recently from e286aef to b443309 Compare November 23, 2023 15:39
@neriumrevolta neriumrevolta force-pushed the petko/migrate-state-from-json-to-db branch 6 times, most recently from 6413d14 to 21f4300 Compare November 28, 2023 12:36
@neriumrevolta neriumrevolta self-assigned this Nov 28, 2023
@neriumrevolta neriumrevolta marked this pull request as ready for review November 28, 2023 12:40
Copy link
Collaborator

@hopeyen hopeyen 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 overall!

subgraph-radio/src/config.rs Show resolved Hide resolved
subgraph-radio/src/entities/mod.rs Outdated Show resolved Hide resolved
subgraph-radio/src/entities/mod.rs Outdated Show resolved Hide resolved
subgraph-radio/src/entities/mod.rs Outdated Show resolved Hide resolved
subgraph-radio/src/operator/attestation.rs Outdated Show resolved Hide resolved
subgraph-radio/src/entities/mod.rs Outdated Show resolved Hide resolved
subgraph-radio/src/server/model/mod.rs Outdated Show resolved Hide resolved
subgraph-radio/src/server/model/mod.rs Outdated Show resolved Hide resolved
subgraph-radio/src/server/model/mod.rs Outdated Show resolved Hide resolved
@neriumrevolta neriumrevolta force-pushed the petko/migrate-state-from-json-to-db branch 4 times, most recently from 606c0a7 to 1b01f0a Compare December 2, 2023 14:45
@neriumrevolta neriumrevolta force-pushed the petko/migrate-state-from-json-to-db branch 2 times, most recently from f7cbac4 to f4db222 Compare December 5, 2023 02:00
@neriumrevolta neriumrevolta force-pushed the petko/migrate-state-from-json-to-db branch 4 times, most recently from 82ae9b4 to 0e7e083 Compare December 5, 2023 20:09
@neriumrevolta neriumrevolta requested review from hopeyen and removed request for hopeyen December 5, 2023 20:10
@neriumrevolta neriumrevolta force-pushed the petko/migrate-state-from-json-to-db branch from 0e7e083 to 91758b2 Compare December 6, 2023 08:52
Copy link
Collaborator

@hopeyen hopeyen left a comment

Choose a reason for hiding this comment

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

much clean!

also pls link the testing issue we discussed. I also still think cargo tests should use the sqlx::test macro, cleaner testing environment

.github/workflows/gen-binaries.yml Outdated Show resolved Hide resolved
subgraph-radio/benches/attestations.rs Outdated Show resolved Hide resolved
subgraph-radio/src/messages/poi.rs Outdated Show resolved Hide resolved
subgraph-radio/src/operator/attestation.rs Outdated Show resolved Hide resolved
@neriumrevolta neriumrevolta force-pushed the petko/migrate-state-from-json-to-db branch 2 times, most recently from b221996 to 2aa8b44 Compare December 7, 2023 08:59
message_block,
);
trace!("save local attestations: {:#?}", local_attestations);
let timestamp_result: Result<u32, _> = nonce.try_into();
Copy link
Collaborator

Choose a reason for hiding this comment

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

would be good to refactor the processing here back into a similar notion of save_local_attestation(...)

@neriumrevolta neriumrevolta force-pushed the petko/migrate-state-from-json-to-db branch 3 times, most recently from e64263d to d15db21 Compare December 10, 2023 10:39
@neriumrevolta
Copy link
Contributor Author

neriumrevolta commented Dec 10, 2023

much clean!

also pls link the testing issue we discussed. I also still think cargo tests should use the sqlx::test macro, cleaner testing environment

thanks!
I think I addressed all the comments now, I could not get sqlx::test to work even with just the cargo tests, again with the

control attributes are not allowed unless the `migrate` feature is enabled and automatic test DB management is used; see docs

Lmk if it's fine to keep it as is. Also for the е2е tests testing Issue - I am not sure how to frame the improvements we want, I think we can leave it as a separate effort from this Issue, since it's not related to the db?

@hopeyen hopeyen mentioned this pull request Dec 11, 2023
2 tasks
@hopeyen
Copy link
Collaborator

hopeyen commented Dec 11, 2023

I could not get sqlx::test to work even with just the cargo tests, again with the

control attributes are not allowed unless the `migrate` feature is enabled and automatic test DB management is used; see docs

I read through the docs carefully and got cargo tests to work

    #[sqlx::test(migrations = "./migrations")]
    async fn test_db_basic(pool: SqlitePool) {
        // let pool = create_test_db(None).await;
    ...
    }

If you are curious, look more carefully in the docs here talking about signatures

I still think it would be better to create the e2e test re-arch issue and link it here, because the changes in the e2e tests right now is not the ideal which leads to the motivation for that spec'ing work. feel free to leave it a bit more general and fill in later. Some main points are to talk about why there are multiple test binaries and a separate db testing management, what was the limitations of having them within the radio crate, and some functions to be moved into the SDK

Other changes look okay

@neriumrevolta neriumrevolta force-pushed the petko/migrate-state-from-json-to-db branch from 001604a to 1fa1035 Compare December 11, 2023 20:42
Copy link
Collaborator

@hopeyen hopeyen left a comment

Choose a reason for hiding this comment

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

🤞!

@neriumrevolta
Copy link
Contributor Author

@hopeyen I will link the testing Issue later today

@neriumrevolta neriumrevolta merged commit 9ba02ac into dev Dec 12, 2023
7 checks passed
@neriumrevolta neriumrevolta deleted the petko/migrate-state-from-json-to-db branch December 12, 2023 04:41
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.

2 participants