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

[WIP] feat(raiko): enable sqlite db by feature #300

Closed
wants to merge 4 commits into from

Conversation

smtmfft
Copy link
Contributor

@smtmfft smtmfft commented Jun 27, 2024

Try to solve the lifetime issue, solution maybe far away from perfect as the db connection lifetime is hard to handle....

@@ -22,9 +22,13 @@ rand = "0.9.0-alpha.1" # This is an a
rand_chacha = "0.9.0-alpha.1"
tempfile = "3.10.1"
alloy-primitives = { workspace = true, features = ["getrandom"] }

rusqlite = { workspace = true, features = ["trace"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

This should become an optional feature enabled only when the sqlite feature is active

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if later we need to use run-time flag or just using compile-time feature to choose the db impl. but yes, should be linked to optional sqlite now.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should also return the prover row from queries in order to not just return a None. I'll change the queries and function wrappers to do this

@petarvujovic98
Copy link
Contributor

@smtmfft Unrelated to the code, why is this branch on your fork instead of a new branch inside raiko?

@smtmfft
Copy link
Contributor Author

smtmfft commented Jul 1, 2024

@smtmfft Unrelated to the code, why is this branch on your fork instead of a new branch inside raiko?

oh, I get used to use mine when the target is not the main, will change for sure.

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