Remove chain::Id completely
#3173
Conversation
sunce86
changed the title
chain::Id completelychain::Id completely
sunce86
changed the title
chain::Id completelychain::Id completely
I didn't quite get from the description and the code where the hardhat chain id comes from. |
| @@ -290,6 +294,7 @@ impl Error { | |||
| } | |||
| Error::GasPrice(_) => false, | |||
| Error::AccessList(_) => true, | |||
| Error::UnsupportedChain => false, | |||
There was a problem hiding this comment.
A new error variant gets added which can never appear here and yet we have to handle this case. This is a code smell and indicates that these functions should return different errors.
Something for a future refactor, though.
There was a problem hiding this comment.
I think we can drop UnsupportedChain error, as it can appear only during application startup when wrong configuration is used (url parameter) and instead just panic with meaningful message in Rpc::new() function when Chain::try_from() fails.
There was a problem hiding this comment.
I don't have a strong opinion here. If you look at how the same constructor is used in autopilot, you can see that .expect() is used when the Chain::try_from is called in run.rs (so when we are aware of our surrounding and we explicitly want to exit), and error type is used inside Rpc::new because we don't know when and how this function will be called.
But I also agree that this error is rarely hit. If we agree to panic everywhere, then we can remove ChainIdNotSupported error from chain crate altogether. This is dangerous since we might want to try to build the Chain object from unreliable source that could give garbage, but it's not the case now.
There was a problem hiding this comment.
So another option would be to introduce new error type dedicated for Rpc object, for instance RpcError, which will contain only one variant ChainIdNotSupported, and caller of the constructor will decide if want to panic or map error to other type.
Alternatively constructor can return boolean type error as there is only one function which can fail with one error code.
There was a problem hiding this comment.
So another option would be to introduce new error type dedicated for Rpc object, for instance RpcError, which will contain only one variant ChainIdNotSupported, and caller of the constructor will decide if want to panic or map error to other type.
That seems reasonable. @sunce86 do you mind implementing it?
There was a problem hiding this comment.
Sure, I created a new error type
|
| @@ -290,6 +294,7 @@ impl Error { | |||
| } | |||
| Error::GasPrice(_) => false, | |||
| Error::AccessList(_) => true, | |||
| Error::UnsupportedChain => false, | |||
There was a problem hiding this comment.
I think we can drop UnsupportedChain error, as it can appear only during application startup when wrong configuration is used (url parameter) and instead just panic with meaningful message in Rpc::new() function when Chain::try_from() fails.
|
I mean, why do we need It in the code now? Some of the tests use this chain id implicitly? |
Driver tests do. ☝️ |
Description
Matching on
chain::Idis cumbersome because it lacks guarantees about its values. For example,chain::Idcan be constructed from au64orU256without error handling for unsupported chains.So to match properly, we need to first convert
chain::Idtochain::Chain, and then also handle failure of conversion.This proposal suggests replacing
chain::Identirely withchain::Chain, which is already well-encapsulated. This change is feasible because all scenarios wherechain::Chaincannot be constructed are handled appropriately—using an early.expect()whenever a service is restarted.Changes
chain::Idand usechain::ChaininsteadHardhatchain to make driver tests pass.How to test
Existing tests should verify no bugs were introduced.