Give correct error for too high block number#131
Conversation
joepetrowski
left a comment
joepetrowski
left a comment
There was a problem hiding this comment.
Generally LGTM but not caught up on these http-errors so can't say for certain that it's ready. Will let @danforbes take a look too.
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
danforbes
left a comment
danforbes
left a comment
There was a problem hiding this comment.
I don't believe that we can merge this because it breaks our API.
| // Check if the block number is too high | ||
| const { number } = await api.rpc.chain.getHeader(); | ||
| if (blockNumber && number.toNumber() < blockNumber) { | ||
| throw new BadRequest( |
There was a problem hiding this comment.
This is a breaking change because we are now returning a different HTTP response code so I would suggest keeping this until v1.
There was a problem hiding this comment.
I see the argument that it is a breaking change since we introduce a new error that potentially prevents an older error message from appearing. However I think there are two reasons this is ok right now. 1) we have a precedent for introducing these types of error handling changes pre v1 (see #123 as a recent example) and 2) it is replacing downstream error messages that I see as significantly less helpful.
There was a problem hiding this comment.
It's not the error message that is really the problem, it's the error code. This could break integrations if people are expecting a 5xx type error when the block number isn't right.
There was a problem hiding this comment.
What do you think of changing it to a 500 but leaving the rest?
There was a problem hiding this comment.
I think our user group is small enough that we can ask them...
There was a problem hiding this comment.
If we hadn't have already released #123 I would have gone with Zeke's suggestion but given the state of things, I think Joe's is more reasonable.
3 participants
Closes #67
Instead of handling this as its own validation in the middleware pipeline, I realized it makes more sense to do the validation in getHashForBlock. My reasoning is that proper validation requires a call to get the latest block. In getHashForBlock we do several less expensive checks prior and then only check for invalid height if the rpc query getBlockHash fails. Finally, some error catching is added in the get and post wrappers to check for the recently introduced HttpError.