sdk: add StringToKnownChainID function for VAAs #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
3 times, most recently
from
09cb5b4 to
dc21be2
Compare
sdk/vaa/structs.go
Outdated
| } | ||
| } | ||
| 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.
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.
There was a problem hiding this comment.
I’d be okay with that as long as it has a doc string describing what (not) to use it for. Not 100% why they’re called NetworkID and not just ChainID there though. 🤷
There was a problem hiding this comment.
OK awesome, happy to add that documentation.
There was a problem hiding this comment.
I've renamed the function, added documentation, and changed the name of this PR and its description.
johnsaigle
changed the title
Adds a function to the SDK's vaa structs that parses known ChainIDs from string representations of ints or chain names.
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.