-
Notifications
You must be signed in to change notification settings - Fork 1
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
Conversation
7da6287
to
4ebfe76
Compare
e286aef
to
b443309
Compare
6413d14
to
21f4300
Compare
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.
looks good overall!
subgraph-radio/migrations/20231109112858_persisted-state.up.sql
Outdated
Show resolved
Hide resolved
606c0a7
to
1b01f0a
Compare
f7cbac4
to
f4db222
Compare
82ae9b4
to
0e7e083
Compare
0e7e083
to
91758b2
Compare
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.
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
b221996
to
2aa8b44
Compare
subgraph-radio/src/messages/poi.rs
Outdated
message_block, | ||
); | ||
trace!("save local attestations: {:#?}", local_attestations); | ||
let timestamp_result: Result<u32, _> = nonce.try_into(); |
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.
would be good to refactor the processing here back into a similar notion of save_local_attestation(...)
e64263d
to
d15db21
Compare
thanks!
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? |
I read through the docs carefully and got cargo tests to work
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 |
d15db21
to
16e6892
Compare
001604a
to
1fa1035
Compare
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.
🤞!
@hopeyen I will link the testing Issue later today |
Description
&SqlitePool
) wherever we need to update/query the db