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: during dkg generate peer-ids by peer name ordering #4178

Merged
merged 2 commits into from
Feb 1, 2024

Conversation

dpc
Copy link
Contributor

@dpc dpc commented Jan 30, 2024

QoL thing.

@dpc dpc requested a review from a team as a code owner January 30, 2024 19:16
@dpc
Copy link
Contributor Author

dpc commented Jan 30, 2024

Running just mpros three times suggests this work.

Copy link

codecov bot commented Jan 30, 2024

Codecov Report

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

Comparison is base (91e2ef4) 58.14% compared to head (725415b) 58.11%.
Report is 12 commits behind head on master.

Files Patch % Lines
fedimint-server/src/config/api.rs 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4178      +/-   ##
==========================================
- Coverage   58.14%   58.11%   -0.03%     
==========================================
  Files         192      192              
  Lines       43156    43234      +78     
==========================================
+ Hits        25092    25126      +34     
- Misses      18064    18108      +44     

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

@dpc dpc enabled auto-merge January 30, 2024 20:18
@dpc
Copy link
Contributor Author

dpc commented Jan 31, 2024

@douglaz fyi

Comment on lines +665 to +667
// in certain (very obscure) cases, it might be worthwhile to sort by urls, so
// just expose it as an env var; probably no need to document it too much
if std::env::var_os("FM_PEER_ID_SORT_BY_URL").is_some_and(|var| !var.is_empty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Irrespective of this instance not being too important, I feel like we need a more coherent way of managing possible env vars so we can list them in help etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would an enum listing be appropriate?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably not since modules sometimes define env vars too, lots of things to consider.

Copy link
Contributor Author

@dpc dpc Jan 31, 2024

Choose a reason for hiding this comment

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

Let's just move them to one (few, if there are crate-dependency needs) file:

const FM_VAR_BLAH_NAME : &str = "....";

and then we can link in the docs where to find them? Least amount of hassle for the benefit given?

(not doing it in this PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's move discussion to: #4191

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 Feb 1, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 1, 2024
@dpc dpc added this pull request to the merge queue Feb 1, 2024
Merged via the queue into fedimint:master with commit e9a90e2 Feb 1, 2024
21 checks passed
@dpc dpc deleted the 24-01-30-peer-id-qol branch February 1, 2024 06:04
@douglaz
Copy link
Contributor

douglaz commented Feb 9, 2024

Will this be backported?

@fedimint-backports
Copy link

Successfully created backport PR for releases/v0.2:

@dpc
Copy link
Contributor Author

dpc commented Feb 9, 2024

@douglaz done

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.

None yet

5 participants