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.
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
Remove
chain::Id
completely #3173Remove
chain::Id
completely #3173Changes from 5 commits
b56c819
f627e9e
1ab46b4
500b83f
84ce1e4
57c3057
eaa36cf
d643e3c
a058e3b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 inRpc::new()
function whenChain::try_from()
fails.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 opinion here. If you look at how the same constructor is used in
autopilot
, you can see that.expect()
is used when theChain::try_from
is called inrun.rs
(so when we are aware of our surrounding and we explicitly want to exit), and error type is used insideRpc::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 fromchain
crate altogether. This is dangerous since we might want to try to build theChain
object from unreliable source that could give garbage, but it's not the case now.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.
So another option would be to introduce new error type dedicated for
Rpc
object, for instanceRpcError
, which will contain only one variantChainIdNotSupported
, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems reasonable. @sunce86 do you mind implementing 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.
Sure, I created a new error type