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.

Sign up for GitHub

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: simplify k generation following RFC 6979 #570

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

refactor: simplify k generation following RFC 6979 #570

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
128 changes: 97 additions & 31 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion starknet-core/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ all-features = true
[dependencies]
starknet-crypto = { version = "0.7.2", path = "../starknet-crypto", default-features = false, features = ["alloc"] }
base64 = { version = "0.21.0", default-features = false, features = ["alloc"] }
crypto-bigint = { version = "0.5.1", default-features = false }
crypto-bigint = { version = "0.5.1", default-features = false, features = ["generic-array"] }
flate2 = { version = "1.0.25", optional = true }
hex = { version = "0.4.3", default-features = false, features = ["alloc"] }
serde = { version = "1.0.160", default-features = false, features = ["derive"] }
Expand Down
10 changes: 4 additions & 6 deletions starknet-crypto/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,13 @@ exclude = ["test-data/**"]

[dependencies]
starknet-curve = { version = "0.5.1", path = "../starknet-curve" }
crypto-bigint = { version = "0.5.1", default-features = false, features = ["generic-array", "zeroize"] }
hmac = { version = "0.12.1", default-features = false }
num-bigint = { version = "0.4.3", default-features = false }
num-integer = { version = "0.1.45", default-features = false }
num-traits = { version = "0.2.18", default-features = false }
rfc6979 = { version = "0.4.0", default-features = false }
sha2 = { version = "0.10.6", default-features = false }
zeroize = { version = "1.6.0", default-features = false }
rfc6979 = { version = "0.5.0-pre.3", default-features = false }
Copy link
Owner

Choose a reason for hiding this comment

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

Do we have to use prerelease versions? I don't think it's the best idea to depend on prereleases. If we depend on upcoming features then we can wait for a release.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes in order to simplify the code of generate_k and avoid doing some manual stuffs, we need to depend on this up to date version. But rfc6979 is a crate of RustCrypto which is the official Rust crypto library. It is audited and constantly checked and updated. The changes are minor between versions so I don't think there is any risk in doing this. Precisely, the version previously used was revised and corrected later (if I'm not mistaken it dated 3 years ago, which isn't necessarily great either).

Copy link
Owner

Choose a reason for hiding this comment

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

rfc6979 v0.4.0 was out 2023-03-01 tho.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes but I checked when I did the manipulations and what is done here was impossible with this version, meaning that there was a fix in the middle. Feel free to wait for an update is needed but I think this is completely fine to do this with beta version.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah I understand that this is a new feature not available from v0.4.0. That said, I prefer to wait for the release, as we're actually not getting much out of it beyond "our code now looks nicer", which is a nice-to-have that we don't have to rush for IMO.

subtle = { version = "2.6.1", default-features = false }
sha2 = { version = "0.11.0-pre.3", default-features = false }
hex-literal = { version = "0.4.1", default-features = false }
hex = { version = "0.4.3", default-features = false, optional = true }
starknet-types-core = { version = "0.1.6", default-features = false, features = ["curve", "hash"] }

Expand All @@ -35,7 +34,6 @@ signature-display = ["dep:hex", "alloc"]
[dev-dependencies]
criterion = { version = "0.4.0", default-features = false }
hex = "0.4.3"
hex-literal = "0.4.1"
serde = { version = "1.0.160", features = ["derive"] }
serde_json = "1.0.96"
starknet-types-core = { version = "0.1.6", default-features = false, features = ["alloc"] }
Expand Down
Loading
Loading