Skip to content

Assorted chores#166

Merged
piotr-roslaniec merged 12 commits intomainfrom
chores
Jan 18, 2024
Merged

Assorted chores#166
piotr-roslaniec merged 12 commits intomainfrom
chores

Conversation

@piotr-roslaniec
Copy link
Copy Markdown

@piotr-roslaniec piotr-roslaniec commented Oct 31, 2023 •
edited
Loading

Type of PR:

  • Other

Required reviews:

  • 1

What does it do:

  • Fixes a bunch of smaller chores
  • Removes support for deprecated Python versions, adds support for Python 3.12
  • Adds a CI job to compile benchmarks without running them (benchmarks were disabled a while back because there was some issue with report uploading)
  • Removes unused curves (unused code) from the codebase

Issues fixed/closed:

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Oct 31, 2023 •
edited
Loading

Codecov Report

Merging #166 (58002f5) into main (13d9d26) will increase coverage by 0.05%.
The diff coverage is 97.77%.

@@            Coverage Diff             @@
##             main     #166      +/-   ##
==========================================
+ Coverage   77.95%   78.01%   +0.05%     
==========================================
  Files          23       23              
  Lines        5053     5066      +13     
==========================================
+ Hits         3939     3952      +13     
  Misses       1114     1114
Files Coverage Δ
ferveo-common/src/keypair.rs 72.97% <100.00%> (+0.53%) ⬆️
ferveo-tdec/src/ciphertext.rs 97.95% <ø> (ø)
ferveo-tdec/src/combine.rs 98.51% <ø> (ø)
ferveo-tdec/src/context.rs 98.03% <ø> (ø)
ferveo-tdec/src/decryption.rs 70.72% <ø> (ø)
ferveo-tdec/src/hash_to_curve.rs 100.00% <ø> (ø)
ferveo-tdec/src/key_share.rs 32.60% <ø> (ø)
ferveo-tdec/src/lib.rs 99.78% <ø> (ø)
ferveo-tdec/src/secret_box.rs 63.15% <ø> (ø)
ferveo/src/api.rs 88.93% <100.00%> (+0.02%) ⬆️
... and 6 more

... and 1 file with indirect coverage changes

@piotr-roslaniec piotr-roslaniec marked this pull request as ready for review November 2, 2023 09:54
@cygnusv
Copy link
Copy Markdown
Member

cygnusv commented Nov 7, 2023

@piotr-roslaniec what do you think of ferveo-tdec instead of ferveo-tpke? I know that I suggested it, but in retrospective I don't know why I didn't think of that before!

@piotr-roslaniec
Copy link
Copy Markdown
Author

piotr-roslaniec commented Nov 7, 2023

@cygnusv Makes sense, also tpke doesn't quite roll off the tongue. I will update this PR shortly.

cygnusv
cygnusv reviewed Nov 8, 2023
View reviewed changes
ferveo-tdec/Cargo.toml
@@ -1,5 +1,5 @@
[package]
name = "group-threshold-cryptography-pre-release"
name = "ferveo-tdec"
Copy link
Copy Markdown
Member

@cygnusv cygnusv Nov 8, 2023

Choose a reason for hiding this comment

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

Ah, so refreshing!

ferveo-tdec/README.md
Copy link
Copy Markdown
Member

@cygnusv cygnusv Nov 8, 2023

Choose a reason for hiding this comment

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

README still refers to tpke

ferveo-tdec/benches/tpke.rs
black_box, criterion_group, criterion_main, BenchmarkId, Criterion,
};
use group_threshold_cryptography_pre_release::{
use ferveo_tdec::{
Copy link
Copy Markdown
Member

@cygnusv cygnusv Nov 8, 2023

Choose a reason for hiding this comment

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

This remind me of a situation a couple of months ago that was making me crazy when working with Ferveo until I realized (or actually, you pointed out). Even though the package and directory are called ferveo-tdec, the crate is automatically renamed to ferveo_tdec. Do you think it would be worth to rename everything to underscores?

Copy link
Copy Markdown
Author

@piotr-roslaniec piotr-roslaniec Nov 8, 2023

Choose a reason for hiding this comment

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

Using both - and _ as a separator in crate names is fine. There is a debate on setting the naming convention.

I'm fine with renaming all crates, we have to republish them anyway. The directories should remain named according to the kebab-case.

Copy link
Copy Markdown
Author

@piotr-roslaniec piotr-roslaniec Nov 8, 2023

Choose a reason for hiding this comment

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

I've documented this as a todo here: #122

Copy link
Copy Markdown
Member

@cygnusv cygnusv Nov 8, 2023

Choose a reason for hiding this comment

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

The directories should remain named according to the kebab-case

Why?

Copy link
Copy Markdown
Author

@piotr-roslaniec piotr-roslaniec Nov 8, 2023

Choose a reason for hiding this comment

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

Well, I can't cite any specific source on that, but I think it's a cargo project convention. I think, very generally speaking, top-level modules (including *.rs files) are to use kebab-case and non-top-level files (never directories) are to use snake_case.

@piotr-roslaniec piotr-roslaniec mentioned this pull request Jan 10, 2024
Closed
@piotr-roslaniec piotr-roslaniec mentioned this pull request Jan 18, 2024
Merged
@piotr-roslaniec piotr-roslaniec merged commit 7350d91 into main Jan 18, 2024
@piotr-roslaniec piotr-roslaniec deleted the chores branch January 18, 2024 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cygnusv cygnusv cygnusv left review comments

@theref theref theref approved these changes

@KPrasch KPrasch Awaiting requested review from KPrasch

@derekpierre derekpierre Awaiting requested review from derekpierre

@manumonti manumonti Awaiting requested review from manumonti

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Delete references to ed25519 Rename group_threshold_cryptography crate to ferveo-tpke Derive Eq for DkgPublicKey Add .pyi file linting

4 participants

@piotr-roslaniec @codecov-commenter @cygnusv @theref