-
Notifications
You must be signed in to change notification settings - Fork 499
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
xdr: get address from AccountId without panic #1999
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the new GetAddress
function will still panic in some cases, and if we're making the new function an implementation that we can assume won't panic, we should return those errors too.
xdr/account_id.go
Outdated
if aid == nil { | ||
return "" | ||
return "", nil | ||
} | ||
|
||
switch aid.Type { | ||
case PublicKeyTypePublicKeyTypeEd25519: | ||
ed := aid.MustEd25519() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line could still panic if the key isn't actually an Ed25519 key or is corrupted in such a way that this fails. If you want to eradicate all panics here you can replace this with GetEd25519
which will return the same result plus an ok
bool
.
xdr/account_id.go
Outdated
} | ||
|
||
switch aid.Type { | ||
case PublicKeyTypePublicKeyTypeEd25519: | ||
ed := aid.MustEd25519() | ||
raw := make([]byte, 32) | ||
copy(raw, ed[:]) | ||
return strkey.MustEncode(strkey.VersionByteAccountID, raw) | ||
return strkey.MustEncode(strkey.VersionByteAccountID, raw), nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same with this line. strkey.Encode
will get you the same behavior without a panic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good. It's great to offer an alternative to panics here. Thanks for taking the initiative to improve this.
I mentioned @ire-and-curses as an FYI because I feel like he might have an opinion on the naming, but I think the naming is making the best of an imperfect situation.
|
||
// GetAddress returns the strkey encoded form of this AccountId, and an error | ||
// if the AccountId is backed by a public key of an unknown type. | ||
func (aid *AccountId) GetAddress() (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ire-and-curses I know you like to think about this stuff: Ideally this would be Address
and the existing function would be MustAddress
, but there's no way we can do this in a backwards compatible way. Calling this GetAddress
looks like the best approach here. FYI in case you're thinking about a holistic clean up of panic/non-panic function names in a future major release.
What
This PR adds a method to the
xdr.AccountId
struct that returns the address with an error. The existingAddress
method is now a thin wrapper around this method.Why
In long-running, data-intensive applications, like the Hubble project, we want to avoid panics wherever possible. A
panic
on a single malformed input can abort the entire pipeline. A panic-free method to get the address of anxdr.AccountId
struct helps reduce such possible failures.Known limitations
N/A