Optimizing crypto/ed25519.go
#21
Conversation
1. Avoid unnecessary copying of byte arrays.
it is not necessary to make a copy of the byte array in these cases since the byte arrays are the same size and have the same underlying structure.
2. Use of constants instead of literals `const CompressedPublicKeySize = 33` instead `if len(paddr) != 33`
crypto/ed25519.go
Outdated
| var pk PublicKey | ||
| copy(pk[:], rpk) | ||
| return pk | ||
| return PublicKey(p[32:]) |
There was a problem hiding this comment.
Very nice...not sure why I didn't do this in the first place.
You should use PublicKeyLen here instead of 32. I also think you should keep the commend that explains why we go 32 in.
There was a problem hiding this comment.
Done! Thank you!
- Moved constant definition top - Using `PublicKeyLen` instead of `32`
|
Ahhh that's why I didn't use it. Is this feature new in go1.20 (tests don't run on that yet)? I agree much cleaner (nice find)...may just need to update the versions rq first. Update: yeah looks like this added it golang/go#46505. Update 2: we'll be able to remove a TON of memory allocations using this. creating an issue now to apply this change all over the codebase (#23). |
- Simplifying the function isn't possible, I thought it was but checking it on a text editor, I realized is not possible.
crypto/ed25519.go
Outdated
| @@ -75,7 +75,10 @@ func GeneratePrivateKey() (PrivateKey, error) { | |||
| // PublicKey returns a PublicKey associated with the Ed25519 PrivateKey p. | |||
| // The PublicKey is the last 32 bytes of p. | |||
| func (p PrivateKey) PublicKey() PublicKey { | |||
| return PublicKey(p[PublicKeyLen:]) | |||
| rpk := p[PrivateKeyLen:] // privateKey == private|public | |||
There was a problem hiding this comment.
You can put a TODO here for #23 so we make sure to catch it.
There was a problem hiding this comment.
Or just leave it. LGTM now.
There was a problem hiding this comment.
Added the PrivateKeyLen variable to be more explicit, this MR is just some minor clean-ups.
|
🤦 lol forgot about this...sigh.
So question is do we want a separate const for "private key component" of privatekey. Looking in the |
|
Ahh they call this the So let's add a constant called |
Yeah sorry about that, I'm fairly new with Go, and I didn't check for backward compatibility. |
You've actually been able to do this since go1.17 but the old style was super gross, so I stayed away. But 1.20 makes it MUCH better: |
|
Should I define a |
You should define |
|
Note the |
- Updating naming convention - Fixing wrong variable naming (`PrivateKeyLen` (64) to `PublicKeyLen` (32))
Consistency
|
@patrick-ogrady I don't understand why are we defining |
| @@ -74,7 +76,7 @@ func GeneratePrivateKey() (PrivateKey, error) { | |||
| // PublicKey returns a PublicKey associated with the Ed25519 PrivateKey p. | |||
| // The PublicKey is the last 32 bytes of p. | |||
| func (p PrivateKey) PublicKey() PublicKey { | |||
| rpk := p[32:] // privateKey == private|public | |||
There was a problem hiding this comment.
Put PrivateKeySeedLen here ^^
|
Found a few erroneous comments and am going to do a pass on this after I merge it as well 👍 . |
|
@rafael-abuawad Going to merge this and make the lint fixes myself. Thanks for putting this up! |
|
Continues here: #26 |
|
Congrats on being the first external contributor! https://twitter.com/_patrickogrady/status/1628870022360895488 |
Avoid unnecessary copying of byte arrays, it is not necessary to make a copy of the byte array in these cases since the byte arrays are the same size and have the same underlying structure.
Use of constants instead of literals To make it more declarative
const CompressedPublicKeySize = 33insteadif len(paddr) != 33