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

chore: Client Database Migrations (without state machine migrations) #4103

Merged
merged 4 commits into from
Jan 30, 2024

Conversation

m1sterc001guy
Copy link
Contributor

Step 1 of #3417

This PR adds the equivalent tests and functions for client db migrations that the server side has.

Best to review this PR per commit, each module is a separate commit.

State machine db migrations will come next.

Copy link

codecov bot commented Jan 23, 2024

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (ba49027) 58.36% compared to head (7ee0d4a) 58.16%.
Report is 47 commits behind head on master.

Files Patch % Lines
fedimint-client/src/lib.rs 90.90% 2 Missing ⚠️
modules/fedimint-dummy-client/src/db.rs 75.00% 2 Missing ⚠️
modules/fedimint-dummy-client/src/lib.rs 75.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4103      +/-   ##
==========================================
- Coverage   58.36%   58.16%   -0.21%     
==========================================
  Files         192      192              
  Lines       42646    43034     +388     
==========================================
+ Hits        24890    25029     +139     
- Misses      17756    18005     +249     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

fedimint-cli/src/lib.rs Outdated Show resolved Hide resolved
fedimint-client/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@dpc dpc left a comment

Choose a reason for hiding this comment

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

Generally LGTM, with some minor comments.

One thing that it's not doing is touching active executor states. Do you think it's not neccessary (because meta on it is serialized in json, so it can be somewhat gradually handled?)

Video of my review: https://youtu.be/IwAWRa889jA

@m1sterc001guy
Copy link
Contributor Author

Thanks for the review!

One thing that it's not doing is touching active executor states. Do you think it's not neccessary (because meta on it is serialized in json, so it can be somewhat gradually handled?)

Nope, I think this does need to be handled. I just wanted to do it in a separate PR since this one was rather big already.

@dpc
Copy link
Contributor

dpc commented Jan 24, 2024

Big recovery refactor landed, good time to rebase and land.

@m1sterc001guy
Copy link
Contributor Author

I added the MintRecoveryState to the mint migration test and did my best to populate as much of it as possible. I had to make some things public that were previously only within the crate or private, hopefully this is an acceptable tradeoff for having migration coverage

elsirion
elsirion previously approved these changes Jan 26, 2024
Copy link
Contributor

@elsirion elsirion left a comment

Choose a reason for hiding this comment

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

Not the cleanest commit history, but an important step towards client version migrations 🎉

fedimint-client/src/module/init.rs Show resolved Hide resolved
@@ -1,4 +1,4 @@
mod db;
pub mod db;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok with making this pub, but is there a reason for migration tests not to live in their respective crate (client or server)? We mainly have the tests crates for integration-testing client and server together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally did it just so all of the migration tests were in the same spot, but now I'm somewhat regretting it as it made me change all of these things to pub. Would you prefer if I moved them back? Probably best done in a separate PR

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it's not worth uprooting this PR. Let's try to tidy up public interfaces in a separate PR. Not super high priority.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will leave this for now and handle in separate PR

modules/fedimint-ln-tests/tests/tests.rs Show resolved Hide resolved
crypto/tbs/src/lib.rs Outdated Show resolved Hide resolved
modules/fedimint-mint-tests/tests/tests.rs Show resolved Hide resolved
@elsirion elsirion added this pull request to the merge queue Jan 30, 2024
Merged via the queue into fedimint:master with commit 91e2ef4 Jan 30, 2024
20 of 21 checks passed
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.

None yet

4 participants