-
Notifications
You must be signed in to change notification settings - Fork 4
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
Create constants for suported network names #39
Conversation
✅ Heimdall Review Status
|
pkg/coinbase/utils.go
Outdated
EthereumHolesky = "ethereum-holesky" | ||
EthereumMainnet = "ethereum-mainnet" | ||
|
||
SolanaDevnet = "solana-devnet" |
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.
should this be string(client.NETWORKIDENTIFIER_SOLANA_DEVNET) ?
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.
I don't have a strong preference here, but I was following the same convention from the NodeJS constant setup. It uses strings and does not link back to the API enums.
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.
I see the mapping happen here
Basically I would like to rely on the server side constants to define what the network ids need to look like instead of us defining it again here.
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.
Ah I misread that syntax. Yeah you're right. So then we may not need them entirely, however, the way the constants are setup in the generate files, it creates an all caps. So I'll keep them in and do what you suggested initially.
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.
Yup - the all caps thing is not great - plus its like a separate type - all our functions need a string networkID so doing string(client.NETWORKIDENTIFIER_SOLANA_DEVNET)
makes the most sense to me.
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.
I like this idea 👍. I think Rohit's idea to limit the amount of "redefining" we do requires much less maintenance moving forward.
What changed? Why?
Qualified Impact