Skip to content
This repository was archived by the owner on Nov 6, 2020. It is now read-only.

Fix filter_block_not_found error message#9359

Closed
sorpaas wants to merge 2 commits into
masterfrom
sp-filter-block-error-message
Closed

Fix filter_block_not_found error message#9359
sorpaas wants to merge 2 commits into
masterfrom
sp-filter-block-error-message

Conversation

@sorpaas
Copy link
Copy Markdown
Collaborator

@sorpaas sorpaas commented Aug 15, 2018

Address grumble #9256 (review)
cc @jimpo

@sorpaas sorpaas added A0-pleasereview 🤓 Pull request needs code review. M6-rpcapi 📣 RPC API. labels Aug 15, 2018
@sorpaas sorpaas added this to the 2.1 milestone Aug 15, 2018
pub fn filter_block_not_found(id: BlockId) -> Error {
Error {
code: ErrorCode::ServerError(codes::UNSUPPORTED_REQUEST), // Specified in EIP-234.
message: "One of the blocks specified in filter (fromBlock, toBlock or blockHash) cannot be found".into(),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer having a separate message for that error, something like that:

pub fn filter_block_error(id: BlockId, msg: &str) -> Error {
	Error {
		code: ErrorCode::ServerError(codes::UNSUPPORTED_REQUEST), // Specified in EIP-234.
		message: msg.into(),
		data: Some(Value::String(match id {
			BlockId::Hash(hash) => format!("0x{:x}", hash),
			BlockId::Number(number) => format!("0x{:x}", number),
			BlockId::Earliest => "earliest".to_string(),
			BlockId::Latest => "latest".to_string(),
		})),
	}
}
pub fn filter_block_not_found(id: BlockId) -> Error {
	filter_block_error(id, "One of ...")
}
pub fn filter_block_out_of_order(id: BlockId) -> Error {
	filter_block_error(id, "fromBlock and toBlock are out of order")
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs a custom error type in ethcore, and we cannot use ? syntax any more for the Client::logs function. Not sure it would worth it or not, but let me give a try.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would indeed need some amounts of change in Client error types, or we would need to duplicate the checks in Client::logs in RPC again (which I don't think is a good idea). For the former, I think it may be better to fix this together with a Client and Blockchain struct error type overhaul, so that we can preserve using ? syntax there. So I'm going to temporarily close this.

I created an issue #9373.

@ordian ordian added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 16, 2018
@sorpaas sorpaas closed this Aug 17, 2018
@5chdn 5chdn deleted the sp-filter-block-error-message branch November 27, 2018 11:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. M6-rpcapi 📣 RPC API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants