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: refactor Client initialization #3918

Merged
merged 3 commits into from
Dec 13, 2023

Conversation

dpc
Copy link
Contributor

@dpc dpc commented Dec 12, 2023

Fix #3748

  • split the 3 cases of initalization (to be built on later)
  • require only needed args, instead of using Option gymnastics

@dpc
Copy link
Contributor Author

dpc commented Dec 12, 2023

Need to add comments and fix restore test which will now require an empty db.

Copy link

codecov bot commented Dec 12, 2023

Codecov Report

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

Comparison is base (9b906d2) 57.05% compared to head (e0a081e) 57.12%.

Files Patch % Lines
fedimint-cli/src/lib.rs 0.00% 115 Missing ⚠️
devimint/src/federation.rs 0.00% 49 Missing ⚠️
devimint/src/main.rs 0.00% 34 Missing ⚠️
fedimint-client/src/lib.rs 75.18% 33 Missing ⚠️
fedimint-wasm-tests/src/lib.rs 0.00% 27 Missing ⚠️
fedimint-load-test-tool/src/common.rs 0.00% 21 Missing ⚠️
gateway/ln-gateway/src/client.rs 78.12% 7 Missing ⚠️
fedimint-cli/src/client.rs 0.00% 2 Missing ⚠️
fedimint-cli/src/db_locked.rs 0.00% 2 Missing ⚠️
fedimint-client/src/backup.rs 0.00% 1 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3918      +/-   ##
==========================================
+ Coverage   57.05%   57.12%   +0.06%     
==========================================
  Files         193      193              
  Lines       42938    42930       -8     
==========================================
+ Hits        24499    24524      +25     
+ Misses      18439    18406      -33     

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

@dpc dpc marked this pull request as ready for review December 12, 2023 23:36
@dpc dpc requested review from a team as code owners December 12, 2023 23:36
@dpc dpc enabled auto-merge December 13, 2023 00:05
@dpc dpc force-pushed the 23-12-11-client-refactor branch 2 times, most recently from 69c691f to e53bcfa Compare December 13, 2023 00:36
@dpc dpc force-pushed the 23-12-11-client-refactor branch 2 times, most recently from c89e38a to 133acda Compare December 13, 2023 03:48
Fix fedimint#3748

* split the 3 cases of initalization (to be built on later)
* require only needed args, instead of using gimnastics
Restoring client always require a new clean db now. Wiping is
supa-clunky.
@dpc dpc requested a review from douglaz December 13, 2023 18:14
Copy link
Contributor

@douglaz douglaz left a comment

Choose a reason for hiding this comment

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

LGTM

@dpc dpc added this pull request to the merge queue Dec 13, 2023
@douglaz
Copy link
Contributor

douglaz commented Dec 13, 2023

Should we backport this?

@dpc
Copy link
Contributor Author

dpc commented Dec 13, 2023

Added a tag, we'll see if it works and can decide then.

Merged via the queue into fedimint:master with commit bc0d392 Dec 13, 2023
22 checks passed
@dpc dpc deleted the 23-12-11-client-refactor branch December 13, 2023 19:26
@fedimint-backports
Copy link

Backport failed for releases/v0.2, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin releases/v0.2
git worktree add -d .worktree/backport-3918-to-releases/v0.2 origin/releases/v0.2
cd .worktree/backport-3918-to-releases/v0.2
git switch --create backport-3918-to-releases/v0.2
git cherry-pick -x 3746d51dc6d060ed8a10662a2ccdd065a501160e 742c84a71410e3fb4ab5f0170580f1047c1d2f93 e0a081e9234b5d2e4a418d16c2b026414a333379

@github-actions github-actions bot mentioned this pull request Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fedimint-client Builder is error-prone
3 participants