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

x/crypto/curve25519: consider removing dependency on "fmt" if worthwhile #48154

Open
dmitshur opened this issue Sep 2, 2021 · 4 comments
Open

x/crypto/curve25519: consider removing dependency on "fmt" if worthwhile #48154

dmitshur opened this issue Sep 2, 2021 · 4 comments
Labels
Milestone

Comments

@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Sep 2, 2021

The curve25519 package imports fmt and uses it 3 times:

return nil, fmt.Errorf("bad scalar length: %d, expected %d", l, 32)
return nil, fmt.Errorf("bad point length: %d, expected %d", l, 32)
return nil, fmt.Errorf("bad input point: low order point")

If it makes sense to remove fmt, then the golang.org/x/crypto/curve25519 can be moved in go/build.TestDependencies from:

# CRYPTO-MATH is core bignum-based crypto - no cgo, net; fmt now ok.

To:

# CRYPTO is core crypto algorithms - no cgo, fmt, net.

Similarly to how crypto/ed25519/internal/edwards25519 was moved in CL 276272.

@FiloSottile, do you think doing so would be worthwhile?

@dmitshur dmitshur added this to the Backlog milestone Sep 2, 2021
@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Sep 2, 2021

Yeah, why not, those can be simple strconv.Itoa and errors.New calls. I'll send a CL.

@dmitshur
Copy link
Contributor Author

@dmitshur dmitshur commented Sep 2, 2021

Sounds good. Please include a small test in x/crypto that this package doesn't import fmt, so this property doesn't accidentally regress without the benefit of go/build's test.

@dmitshur dmitshur removed this from the Backlog milestone Sep 2, 2021
@dmitshur dmitshur added this to the Go1.18 milestone Sep 2, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Sep 7, 2021

Change https://golang.org/cl/348069 mentions this issue: x/crypto/curve25519: remove dependency on "fmt"

@aaqaishtyaq
Copy link

@aaqaishtyaq aaqaishtyaq commented Sep 7, 2021

Hey @FiloSottile, I would like to work on this. I have removed dependency on fmt from curve25519/curve25519.go.
Let me know if I am going right and additional changes I need to make.

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants