fedimint / fedimint

Federated E-Cash Mint
https://fedimint.org/
MIT License
536 stars 210 forks source link

chore: cleanup imports and exports in fedimint-core and fedimint-server #3296

Closed tvolk131 closed 7 months ago

codecov[bot] commented 7 months ago

Codecov Report

All modified lines are covered by tests :white_check_mark:

Comparison is base (eb46134) 60.15% compared to head (24ea195) 60.11%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #3296 +/- ## ========================================== - Coverage 60.15% 60.11% -0.04% ========================================== Files 193 193 Lines 40355 40363 +8 ========================================== - Hits 24276 24265 -11 - Misses 16079 16098 +19 ``` | [Files](https://app.codecov.io/gh/fedimint/fedimint/pull/3296?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fedimint) | Coverage Δ | | |---|---|---| | [fedimint-core/src/core.rs](https://app.codecov.io/gh/fedimint/fedimint/pull/3296?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fedimint#diff-ZmVkaW1pbnQtY29yZS9zcmMvY29yZS5ycw==) | `10.84% <ø> (+0.04%)` | :arrow_up: | | [fedimint-core/src/core/server.rs](https://app.codecov.io/gh/fedimint/fedimint/pull/3296?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fedimint#diff-ZmVkaW1pbnQtY29yZS9zcmMvY29yZS9zZXJ2ZXIucnM=) | `95.23% <ø> (ø)` | | | [fedimint-core/src/module/mod.rs](https://app.codecov.io/gh/fedimint/fedimint/pull/3296?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fedimint#diff-ZmVkaW1pbnQtY29yZS9zcmMvbW9kdWxlL21vZC5ycw==) | `86.80% <ø> (ø)` | | | [fedimint-server/src/config/distributedgen.rs](https://app.codecov.io/gh/fedimint/fedimint/pull/3296?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fedimint#diff-ZmVkaW1pbnQtc2VydmVyL3NyYy9jb25maWcvZGlzdHJpYnV0ZWRnZW4ucnM=) | `89.35% <ø> (ø)` | | | [fedimint-server/src/config/mod.rs](https://app.codecov.io/gh/fedimint/fedimint/pull/3296?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fedimint#diff-ZmVkaW1pbnQtc2VydmVyL3NyYy9jb25maWcvbW9kLnJz) | `85.37% <ø> (-1.30%)` | :arrow_down: | | [fedimint-server/src/consensus/mod.rs](https://app.codecov.io/gh/fedimint/fedimint/pull/3296?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fedimint#diff-ZmVkaW1pbnQtc2VydmVyL3NyYy9jb25zZW5zdXMvbW9kLnJz) | `96.42% <ø> (ø)` | | | [fedimint-server/src/db.rs](https://app.codecov.io/gh/fedimint/fedimint/pull/3296?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fedimint#diff-ZmVkaW1pbnQtc2VydmVyL3NyYy9kYi5ycw==) | `47.66% <ø> (ø)` | | | [fedimint-server/src/lib.rs](https://app.codecov.io/gh/fedimint/fedimint/pull/3296?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fedimint#diff-ZmVkaW1pbnQtc2VydmVyL3NyYy9saWIucnM=) | `82.08% <ø> (+0.57%)` | :arrow_up: | | [fedimint-server/src/multiplexed.rs](https://app.codecov.io/gh/fedimint/fedimint/pull/3296?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fedimint#diff-ZmVkaW1pbnQtc2VydmVyL3NyYy9tdWx0aXBsZXhlZC5ycw==) | `88.88% <ø> (ø)` | | | [fedimint-server/src/net/api.rs](https://app.codecov.io/gh/fedimint/fedimint/pull/3296?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fedimint#diff-ZmVkaW1pbnQtc2VydmVyL3NyYy9uZXQvYXBpLnJz) | `80.88% <ø> (-0.20%)` | :arrow_down: | | ... and [2 more](https://app.codecov.io/gh/fedimint/fedimint/pull/3296?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fedimint) | | ... and [5 files with indirect coverage changes](https://app.codecov.io/gh/fedimint/fedimint/pull/3296/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fedimint)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

elsirion commented 7 months ago

Can we somehow enforce this in CI? Otherwise such changes are close to pointless since we'll regress again over time.

dpc commented 7 months ago

@tvolk131 Do you want to try enforcing it with .semgrep.yaml?

douglaz commented 7 months ago

Try to enforce this using clippy. For instance, enable: https://rust-lang.github.io/rust-clippy/master/index.html#/wildcard_imports

joschisan commented 7 months ago

Please hold off on merging this pr as the aleph pr introduces a ton off changes in fedimint-server and any changes may send me into rebase hell…

justinmoon commented 7 months ago

Please hold off on merging this pr as the aleph pr introduces a ton off changes in fedimint-server and any changes may send me into rebase hell…

I just marked this PR as draft until https://github.com/fedimint/fedimint/pull/3260 is merged

dpc commented 7 months ago

@tvolk131 You might want to submit a commit without fedimint-server changes.

tvolk131 commented 7 months ago

Do you want to try enforcing it with .semgrep.yaml?

Good idea! I messed around a bit and wasn't able to find a great one-size-fits-all regex to only allow wildcard imports where we want it and not where we don't. For example, it's normal to have use super::*; in the test module to avoid re-importing everything that the prod code is relying on. I settled on ^use .*::\*;$ which allows for the aforementioned case since it only counts uses that start with no whitespace (which will exclude any uses in a module declared within a file). It's not perfect, but it prevents regressions on several of the fixes in this PR.

joschisan commented 7 months ago

Please still hold off on merging this pr as I am currently working on a big refactor of the server and this will cause rebase conflicts.

dpc commented 7 months ago

What's the status? LGTM.

dpc commented 7 months ago

image

Is it ready to review and land?

tvolk131 commented 7 months ago

Is it ready to review and land?

@joschisan wanted to hold off on submission

Please still hold off on merging this pr as I am currently working on a big refactor of the server and this will cause rebase conflicts.

@joschisan can you please link the PR we're waiting on so I know when I should send this out for review?

dpc commented 7 months ago

AFAIK it landed a week ago or more. :D

actuallly the last refactor was merged a couple hours before this.