-
Notifications
You must be signed in to change notification settings - Fork 7
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
[TRIVIAL] Base: Add support for Balancer #90
Conversation
src/infra/dex/balancer/dto.rs
Outdated
@@ -189,6 +189,7 @@ impl Chain { | |||
eth::ChainId::Mainnet => Ok(Self::Mainnet), | |||
eth::ChainId::Gnosis => Ok(Self::Gnosis), | |||
eth::ChainId::ArbitrumOne => Ok(Self::Arbitrum), | |||
eth::ChainId::Base => Ok(Self::Base), | |||
unsupported => Err(Error::UnsupportedChainId(unsupported)), |
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.
Instead of using unsupported
as the catch-all term we should probably explicitly list the unsupported networks. That way the compiler would have complained about the match statement here when you added the Base
variant.
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.
This check was made for each request. So, I changed the parsing to be done at startup instead that for each request (which is inefficient). Now it should panic at startup if the network is not supported instead than throwing an error for each request.
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.
The actual comment was not addressed, though.
My point was to change the match statement to ensure that the compiler yells at us whenever we add a new chain. So in our case:
eth::ChainId::Goerli => Err(Error::UnsupportedChain(chain_id)),
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.
Oh, sorry, I totally misunderstood you. On it!
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.
@MartinquaXD done! let me also know if you agree with the changes of making it as a startup error instead of parsing it for each request.
918d9a8
to
5cfce9b
Compare
Add support for Balancer chain ID.
Add support for Balancer chain ID.