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
fix: accept ethlive as a chain name #2268
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,6 +47,7 @@ pub type ParseChainError = TryFromPrimitiveError<Chain>; | |
#[strum(serialize_all = "kebab-case")] | ||
#[repr(u64)] | ||
pub enum Chain { | ||
#[strum(serialize = "ethlive")] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this will change replace "mainnet" whith ethlive. imo mainnet is more meaningful. we could add this as alias, but dunno how with strum tbh. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah it's weird, I learnt this not too long ago: you can have only one "to_string" attr, which is the main one. Then aliases use serialize = ... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I checked cast chain, and the reason we use ethlive is because of eth classic |
||
Mainnet = 1, | ||
Morden = 2, | ||
Ropsten = 3, | ||
|
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.
strum is weird and you have to 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.
That's right, the proposed PR breaks the string representation. I've fixed it by adding another
serialize
parameter, it matches what other enum variants do. I've added tests checking that, but I see that in #2270 you've solved the tests coverage once and for all, so I'll remove them.