sdk: add Atoi function for ChainID (parsing ChainID from string representations of integers) #4239
Conversation
johnsaigle
requested review from
bruce-riley,
evan-gray,
panoel and
SEJeff
as code owners
johnsaigle
changed the title
johnsaigle
force-pushed
the
vaa-atoi
branch
2 times, most recently
from
974c0eb to
09cb5b4
Compare
| } | ||
| } | ||
| if !found { | ||
| err = fmt.Errorf("value %s is not a valid chainId", s) |
There was a problem hiding this comment.
Is it worth doing a special check for id == 0 and returning ChainIDUnset?
There was a problem hiding this comment.
Curious about your thoughts on that actually. I originally had the function set up to do just that, but then since I saw that GetAllNetworkIDs() seemed to specifically omit ChainIDUnset, I figured I'd follow what the existing code was doing. Maybe it's not a problem if they have different behaviours though?
There was a problem hiding this comment.
It depends on the use case. chain ID 0 / unset is used for governance. so if you're using this to parse something that comes from the payload of real VAAs, then it's possible (though I'm not sure why you would have that as a string). If this is more for splitting a VAA ID or something, there should never be a VAA emitted with chain ID 0 as far as I am aware, which is similarly why 0 is not a "Network ID" for GetAllNetworkIDs. I guess I'm not sure of the use case that led to wishing for this 😅
There was a problem hiding this comment.
Follow up - what is the case where you have a chain ID (for some reason) but should be told it is invalid? Any uint16 is technically valid as a chain ID, so anything that passes the first line shouldn't necessarily trigger an error. Otherwise the caller would need to update their code any time there is a new chain added, which quickly becomes a nightmare. Maybe that's why I would lean on the side of continuing to not have this function.
There was a problem hiding this comment.
Hey @evan-gray thanks for the review and for the questions. Essentially my motivation here is to get more use out of the ChainID type and wanted a way to go from a string to a valid, registered ChainID within Wormhole. Based on your questions I'm realizing that I was thinking of this code as a "wormhole SDK" rather than a "vaa SDK" and that's perhaps stretching the purpose of the code.
I think you're right that any uint16 is a valid ChainID in a sense (even if it doesn't correspond to any actual instantiated chain); for that reason Atoi is not a good name here because that sort of generic conversion should accept any uint16, I feel.
Would you be open to including this function if renamed/redocumented to something like "ValidChainIDFromString"? I would personally find this function useful but if in your view this is outside of the scope of this file then I'll defer to your judgement and close the PR.
There was a problem hiding this comment.
My primary hesitation is that it’s a good way to cause a bug where the written code cannot withstand new chain deployments without being upgraded. This is a consistent issue with other tooling. It’s less of an issue in the guardian code (which dictates new chains, at least today) than it is for code elsewhere. Semantically, a chain number is no less “valid” just because a given version of the code doesn’t know what named chain it corresponds to yet.
There was a problem hiding this comment.
Perhaps “known” would be a better adjective.
There was a problem hiding this comment.
How about KnownNetworkIDFromString()? That way we surface that it's using the NetworkID list under the hood and it becomes more clear that 0/ChainIDUnset is not a valid use of the function. In this way also it should have the same maintenance requirements/constraints that GetAllNetworkIDs has.
Adds a function to the SDK's vaa structs that parses valid ChainIDs from string representations of ints.
I've found myself wishing that this function exists in the context of working with ChainIDs. It's nice to have this parsing and validation provided by the SDK.
(Naming comes from https://pkg.go.dev/strconv#Atoi)