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

refactor: remove unused error paths, use expect when possible #4154

Merged
merged 1 commit into from
Jan 30, 2024

Conversation

joschisan
Copy link
Contributor

No description provided.

@joschisan joschisan requested review from a team as code owners January 28, 2024 07:38
Copy link

codecov bot commented Jan 28, 2024

Codecov Report

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

Comparison is base (a099fff) 58.30% compared to head (dfff38d) 58.04%.
Report is 25 commits behind head on master.

Files Patch % Lines
modules/fedimint-mint-client/src/output.rs 86.84% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4154      +/-   ##
==========================================
- Coverage   58.30%   58.04%   -0.26%     
==========================================
  Files         192      192              
  Lines       42757    42972     +215     
==========================================
+ Hits        24928    24942      +14     
- Misses      17829    18030     +201     

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

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.

Great refactor!

.tier(&created.amount)
.expect("We obtained this amount from tbs_pks when we created the output");

// this implies that the mint client config's public keys are inconsistent
Copy link
Contributor

Choose a reason for hiding this comment

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

So maybe in the future we can just check config consistency when joining the federation and then expect here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we could

@@ -185,6 +185,14 @@ fn lagrange_multipliers(scalars: Vec<Scalar>) -> Vec<Scalar> {
.collect()
}

pub fn verify_blinded_signature(
Copy link
Contributor

Choose a reason for hiding this comment

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

Some docs might be useful here for everything that isnt familiar with the pairing crypto

let agg_blind_signature = aggregate_signature_shares(
&blinded_signature_shares
.into_iter()
.map(|(peer, share)| (peer.to_usize() as u64 + 1, share))
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 here is just to prevent a peer with id 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need the +1 here so the cryptography checks out.

Copy link
Contributor

Choose a reason for hiding this comment

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

I knew what it was for but also raised an eye brow that there is no fn to convert the peer ID. But that's how I wrote it an eternity ago and Joscha just kept it that way.

Copy link
Contributor

@m1sterc001guy m1sterc001guy left a comment

Choose a reason for hiding this comment

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

I'm not the most well-versed in the mint module but this looks like a good refactor to me

@joschisan joschisan added this pull request to the merge queue Jan 30, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jan 30, 2024
@joschisan joschisan added this pull request to the merge queue Jan 30, 2024
Merged via the queue into fedimint:master with commit 7f9acec Jan 30, 2024
20 of 21 checks passed
@joschisan joschisan deleted the output branch January 30, 2024 19:21
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

3 participants