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

Modularization III-ish #941

Merged
merged 3 commits into from
Nov 19, 2022
Merged

Conversation

elsirion
Copy link
Contributor

This is an attempt at finishing modularization in a way that is quick, will allow external modules soon, but leaves most of the code moving of existing modules for later.

@codecov-commenter
Copy link

codecov-commenter commented Nov 19, 2022

Codecov Report

Base: 64.26% // Head: 65.05% // Increases project coverage by +0.78% 🎉

Coverage data is based on head (6b6116d) compared to base (5d68002).
Patch coverage: 27.58% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #941      +/-   ##
==========================================
+ Coverage   64.26%   65.05%   +0.78%     
==========================================
  Files         112      111       -1     
  Lines       16412    16196     -216     
==========================================
- Hits        10547    10536      -11     
+ Misses       5865     5660     -205     
Impacted Files Coverage Δ
fedimint-api/src/core.rs 75.21% <0.00%> (+2.68%) ⬆️
fedimint-api/src/core/server.rs 0.00% <0.00%> (ø)
fedimint-api/src/lib.rs 85.80% <ø> (ø)
fedimint-api/src/module/mod.rs 94.73% <ø> (ø)
fedimint-client/src/lib.rs 100.00% <ø> (+96.96%) ⬆️
fedimint-core/src/epoch.rs 99.62% <ø> (ø)
fedimint-core/src/outcome.rs 41.93% <ø> (ø)
fedimint-core/src/transaction.rs 98.05% <ø> (ø)
fedimint-server/src/consensus/mod.rs 90.50% <ø> (ø)
fedimintd/src/bin/main.rs 1.21% <0.00%> (+0.02%) ⬆️
... and 20 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@elsirion elsirion force-pushed the 2022-11-modularization branch from cc09e2a to 8977758 Compare November 19, 2022 03:34
.register_async_method(path, move |params, state| {
Box::pin(async move {
let params = params.one::<serde_json::Value>()?;
// Using AssertUnwindSafe here is far from ideal. In theory this means we could
Copy link
Contributor

Choose a reason for hiding this comment

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

BTW. the whole UnwindSafe is one of a few broken things in Rust and possibly will be deprecated altogether. rust-lang/rust#40628

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 think it's a separate issue (created #943), I just copied the old code to adapt it.

type VerificationCache = LightningVerificationCache;

fn module_key(&self) -> fedimint_api::encoding::ModuleKey {
todo!()
Copy link
Contributor

Choose a reason for hiding this comment

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

Intentional or leftover?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

dpc
dpc previously approved these changes Nov 19, 2022
This automatically bridges the old modules into the new, type-erased module system
@elsirion elsirion force-pushed the 2022-11-modularization branch from 8977758 to 6b6116d Compare November 19, 2022 14:52
@elsirion elsirion marked this pull request as ready for review November 19, 2022 15:55
Copy link
Contributor

@justinmoon justinmoon left a comment

Choose a reason for hiding this comment

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

LGTM

@justinmoon justinmoon merged commit e4c713f into fedimint:master Nov 19, 2022
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.

4 participants