-
Notifications
You must be signed in to change notification settings - Fork 728
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
sdk: add Atoi function for ChainID (parsing ChainID from string representations of integers) #4239
Open
johnsaigle
wants to merge
2
commits into
wormhole-foundation:main
Choose a base branch
from
johnsaigle:vaa-atoi
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+104
−0
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Is it worth doing a special check for id == 0 and returning ChainIDUnset?
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.
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 omitChainIDUnset
, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps “known” would be a better adjective.
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.
How about
KnownNetworkIDFromString()
? That way we surface that it's using the NetworkID list under the hood and it becomes more clear that0
/ChainIDUnset is not a valid use of the function. In this way also it should have the same maintenance requirements/constraints that GetAllNetworkIDs has.