feat_: Update alias generated #6342
Conversation
Jenkins BuildsClick to see older builds (48)
|
ulisesmac
force-pushed
the
fix-display-name
branch
from
91989c3 to
efe7105
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6342 +/- ##
===========================================
+ Coverage 59.69% 59.77% +0.08%
===========================================
Files 865 865
Lines 113050 113053 +3
===========================================
+ Hits 67484 67578 +94
+ Misses 37731 37618 -113
- Partials 7835 7857 +22
Flags with carried forward coverage won't be shown. Click here to find out more.
|
ulisesmac
force-pushed
the
fix-display-name
branch
from
3635971 to
07005bb
Compare
ulisesmac
force-pushed
the
fix-display-name
branch
2 times, most recently
from
d912c7d to
4a938d1
Compare
|
@igor-sirotin @ilmotta The comments have been addressed, could you please review again? Thank you for the feedback! |
protocol/contact.go
Outdated
| return abbreviatedKey | ||
| } | ||
| return "" | ||
| func getShortenedCompressedKey(compressedKey string) string { |
There was a problem hiding this comment.
It seems that we can delete this function, which has only one usage and directly use alias.ShortenedCompressedKey(compressedKey)
protocol/identity/alias/generate.go
Outdated
| // Fix a public key without the 0x prefix | ||
| if len(publicKeyString) == multiformat.LegacyKeyLength-2 && publicKeyString[:2] != "0x" { | ||
| fixedPublicKey = "0x" + publicKeyString | ||
| } else { |
There was a problem hiding this comment.
Here you could unnest a little bit and use else if.
There was a problem hiding this comment.
@ulisesmac mate, what was wrong with my suggestion? 😄
// Attempt to fix a maybe public key without the 0x prefix
if len(publicKeyString) >= 2 && publicKeyString[:2] != "0x" {
fixedPublicKey = "0x" + publicKeyString
}
if len(publicKeyString) != multiformat.LegacyKeyLength {
return fmt.Errorf("invalid length of public key")
}This implementation is short and simple.
- Prepend
0xif it's not yet there. No need to check the length of the given string at all at this stage. - Check the string to be exactly 132 characters, already with
0x. If it's not, then we return an error.
Just 2 simple sequential conditions, no need complex conditions nesting here.
There was a problem hiding this comment.
I think I can't take your suggestion as is since there's some unknowns to me with it.
You mentioned your suggestion has 2 sequential checks and no need to nest, but I think we need to nest or we lack some else clauses to make it work.
The code suggested won't fix a publicKeyString passed without the prefix Ox, instead, it will return the invalid key length error due to the always executed second if:
if len(publicKeyString) != multiformat.LegacyKeyLength {
return fmt.Errorf("invalid length of public key")
}So, I took your suggestion as an idea of what to check instead of as working code.
it'd be clearer if you provide the full function impl. because it seems the suggestion (as is) doesn't work, or is it that I am missing a language feature that will make it work? 🤔
(BTW, I'm new to Go, so I might lack some lang. features or preferred patterns)
There was a problem hiding this comment.
The code suggested won't fix a
publicKeyStringpassed without the prefix0x
Ok I see I did a typo there referencing fixedPublicKey, while it's not needed at all.
Actually, I simplified it even more 🙌. New simplifications are:
- Use
strings.HasPrefixto check the prefix - Remove the key length check, as it exists inside
SerializeLegacyKey:
status-go/api/multiformat/utils.go
Lines 73 to 75 in 4a938d1
Here's the full function:
func GenerateFromPublicKeyString(publicKeyString string) (string, error) {
// Ensure there's `0x` prefix
if strings.HasPrefix(publicKeyString, "0x") {
publicKeyString = "0x" + publicKeyString
}
compressedKey, err := multiformat.SerializeLegacyKey(publicKeyString)
if err != nil {
return "", err
}
return ShortenedCompressedKey(compressedKey), nil
}Explanation
We know that by the point of calling multiformat.SerializeLegacyKey(fixedPublicKey) the public key must match 2 requirements:
- Start with
0xprefix - Be of 132 characters length
Therefore, there's no nesting needed, we just need 2 sequential conditions to satisfy both requirements.
(knowing the 132 length check is inside, we don't need to duplicate it)
@ulisesmac, please let me know if it's still unclear
There was a problem hiding this comment.
Applied, just added a missing !.
Thanks for the suggestion @igor-sirotin
ulisesmac
force-pushed
the
fix-display-name
branch
from
4a938d1 to
1283819
Compare
- Changed from the 3-words name to be the compressed key with an ellipsis - Update the shortened compressed key to be consistent with the alias - Add more tests about the public key length received
ulisesmac
force-pushed
the
fix-display-name
branch
from
1283819 to
c1812f9
Compare
|
@igor-sirotin comments addressed, could you review again please? |
This PR is required to acomplish
The context about this change is mainly in this Discord thread.
Summary
This PR changes the implementation of
GenerateFromPublicKeyStringto return a compressed key with ellipsis instead of the 3-words name.Important changes: