Catch PastHorizon Exception and return 400 with descriptive Error #2059
Conversation
lib/core/src/Cardano/Wallet.hs
Outdated
| liftIO $ ti $ slotRangeFromTimeRange $ Range mStart mEnd | ||
| liftIO (try $ ti $ slotRangeFromTimeRange $ Range mStart mEnd) >>= | ||
| \case | ||
| Left e@(PastHorizon{}) -> throwE (ErrListTransactionsPastHorizonException e) |
There was a problem hiding this comment.
| [ "The end time is past the horizon (due to uncertainty about the next" | ||
| , " hard fork)." | ||
| , " Wait for the node to finish syncing to the hard fork." | ||
| ] |
There was a problem hiding this comment.
Question: Should the eras be listed? (Note that it's a list of Eras)
-- | We tried to convert something that is past the horizon
--
-- That is, we tried to convert something that is past the point in time
-- beyond which we lack information due to uncertainty about the next
-- hard fork.
--
-- We record the condition we were looking for and the bounds on the summary.
data PastHorizonException = PastHorizon CallStack [EraSummary]
data EraSummary = EraSummary {
eraStart :: !Bound -- ^ Inclusive lower bound
, eraEnd :: !EraEnd -- ^ Exclusive upper bound
, eraParams :: !EraParams -- ^ Active parameters
}
deriving stock (Show, Eq, Generic)
deriving anyclass (NoUnexpectedThunks)
|
It'd be better to also provide an integration test for this one although it is quite hard to setup with our current test integration framework (which only executes scenarios once the hard-fork has happened!) |
| @@ -2322,6 +2323,13 @@ instance LiftHandler ErrStartTimeLaterThanEndTime where | |||
| , "'." | |||
| ] | |||
|
|
|||
| instance LiftHandler PastHorizonException where | |||
| handler _ = apiError err400 PastHorizon $ mconcat | |||
| [ "The end time is past the horizon (due to uncertainty about the next" | |||
There was a problem hiding this comment.
The phrase "The end time" is specific to ErrListTransactionsPastHorizonException, whereas this instance could theoretically be thrown from any endpoint, I believe.
How about either:
- Make the the description generic (remove "The end time")
- Define this message directly in the
ErrListTransactionsPastHorizonExceptioncase
There was a problem hiding this comment.
🤔 I think
Define this message directly in the ErrListTransactionsPastHorizonException case
would be preferable.
If a PastHorizonException were to be thrown in many other places, that would be an unexpected server error (some err5xx), different from this case (err400).
(Having a LiftHandler PastHorizonException simply for adding more info than 500 Something wrong in the general unexpected case, could still be good though)
There was a problem hiding this comment.
So, IF PastHorizonException always means that a later request will succeed, then we can just have the LiftHandler instance return 503. Is that the case?
There was a problem hiding this comment.
always means that a later request will succeed, then we can just have the LiftHandler instance return 503. Is that the case?
There should exist cases where it's more like "programmer error", and waiting doesn't really help.
For instance:
When listing stake pools, we convert the retirement epochs to UTCTime:
https://github.com/input-output-hk/cardano-wallet/blob/90c1e5953a48ffc65f004a68abd545ef68574c4b/lib/shelley/src/Cardano/Wallet/Shelley/Pools.hs#L351-L352
In Byron;Shelley;Gougen, before fork to Gougen is confirmed, because the retirements are far in the future, this would likely fail*.
Even if we could wait for the retirement epoch of one pool to enter the forecast range, there could always be additional pools retiring. So waiting won't really help, we need to approach this differently when the time comes.
(*Not a problem currently in Byron;Shelley because pools only exist in Shelley, guaranteeing unlimited forecast)
There was a problem hiding this comment.
Although, I guess "Wait for the node to finish syncing to the hard fork" is still valid in this case, where waiting for Gougen would technically be a solution, and one would notice if it is thrown in an un-resonable context.
hasufell
force-pushed
the
hasufell/1971/better-error-for-byron
branch
from
9e5fda1 to
05bd695
Compare
|
I'm starting to question this is 4xx at all, since the reason the request cannot be fulfilled is that part of the server (the node) isn't ready yet. |
|
So I think 503 is in fact the correct status code: https://stackoverflow.com/a/12281287 |
|
I think 503 might make sense. Scenario: Running Byron;Shelly, when node un-synced in Byron User can wait for either
Only "but" is that the node is not necessarily guaranteed to reach Shelley, for arbitrary chains. Or if we were to ship Byron;Shelley;Gougen weeks before the hard fork to Gougen were confirmed, it would take… weeks to have a unlimited forecast range. |
Good point, but 503 doesn't make any time guarantees, right? It would be nice though if we could give an estimate in the error. Is that possible? |
And adjust the error message slightly.
Co-authored-by: Jonathan Knowles <[email protected]>
|
bors r+ |
2059: Catch PastHorizon Exception and return 400 with descriptive Error r=KtorZ a=hasufell # Issue Number #1971 # Overview - catch the exception and return 400 with descriptive Error # Comments - should this be unit tested? If so, how? Co-authored-by: Julian Ospald <[email protected]> Co-authored-by: Julian Ospald <[email protected]>
Timed out |
|
bors r+ |
Build succeeded |
Issue Number
#1971
Overview
Comments