-
Notifications
You must be signed in to change notification settings - Fork 217
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
Jörmungandr: mkNetworkLayer
with networkTip
#383
Conversation
, nextBlocks = \_slotId -> do
let blockId = undefined
let chunkSize = 1000
ids <- getDescendantIds j blockId chunkSize >>= \case
Just a -> return a
Nothing -> liftIO . throwIO $ ErrGetBlockNotFound blockId
forM ids getBlock'
, postTx = undefined
}
where
getBlock' :: BlockId -> ExceptT ErrNetworkUnreachable m Block
getBlock' t = getBlock j t >>= \case
Just b -> return b
Nothing -> liftIO . throwIO $ ErrGetBlockNotFound t
|
t@(BlockId hash) <- lift $ getTipId j | ||
lift $ getBlock j t >>= \case | ||
(Just b) -> return (hash, header b) | ||
Nothing -> liftIO . throwIO $ ErrGetBlockNotFound t |
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.
Could wrap this in some ErrInternalPanicOrSomething
-wrapper… or handle the error entirely differently
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 adding another constructor to ErrNetworkTip
would be fine.
networkTip
is used to find out the tip slot id.
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.
Good, looks like all that's left in this PR is handling Nothing from getBlock. 👍
t@(BlockId hash) <- lift $ getTipId j | ||
lift $ getBlock j t >>= \case | ||
(Just b) -> return (hash, header b) | ||
Nothing -> liftIO . throwIO $ ErrGetBlockNotFound t |
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 adding another constructor to ErrNetworkTip
would be fine.
networkTip
is used to find out the tip slot id.
@@ -102,7 +102,7 @@ type PostSignedTx | |||
-- TODO: Replace SignedTx with something real | |||
data SignedTx | |||
|
|||
newtype BlockId = BlockId (Hash "block") | |||
newtype BlockId = BlockId (Hash "BlockHeader") |
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.
Yep, I think Hash "BlockHeader"
is correct, and BlockId
probably isn't needed - as either a newtype or type synonym.
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.
It doubles as an ApiT
to avoid orphans so it is needed (in the Api type). I kept it in JormungandrLayer
for now.
Also I introduced it because jormungandr/JormungandrLayer
often refers to "id" in the method-names.
@@ -75,9 +88,9 @@ data JormungandrLayer m = JormungandrLayer | |||
{ getTipId | |||
:: ExceptT ErrNetworkUnreachable m BlockId | |||
, getBlock | |||
:: BlockId -> ExceptT ErrGetBlock m Block | |||
:: BlockId -> ExceptT ErrNetworkUnreachable m (Maybe Block) |
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.
Nothing
denotes that the node is not aware of a block with that hash. Maybe put that in a comment.
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.
As discussed in another review, returning Maybe
here isn't very ideal since in most (all?) cases, we are actually expecting a block when calling getBlock
. Using Maybe
, we are loosing information (hence the need for an extra comment). With a clear error, there's no need for a comment or any doubt about what the behavior of that function is.
May I ask what is nightmarish here? In an |
That would be a much more logical choice in my opinion. We didn't bother with this in the bridge but, for Jormungandr, being more resilient to and more expressive about faults is a good idea. So instead of making Jormungandr internals less expressive, can't we make the network layer more expressive if that makes it easier? |
Yes, without t@(BlockId hash) <- (getTipId j)
`mappingError` ErrNetworkTipNetworkUnreachable
b <- (getBlock j t)
`mappingError` \case
ErrGetBlockNotFound (BlockId h) ->
ErrNetworkTipBlockNotFound h
ErrGetBlockNetworkUnreachable e ->
ErrNetworkTipNetworkUnreachable e
return (hash, header b)
where
mappingError = flip withExceptT I believe
|
15dba9e
to
efc81f2
Compare
efc81f2
to
873f3e6
Compare
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 kept the errors in
JormungandrLayer
now - Some small awkwardness caused from extending
ErrNetworkTip
but I think it should be fine, really
@@ -148,6 +148,8 @@ rbNextBlocks bridge start = maybeTip (getNetworkTip bridge) >>= \case | |||
|
|||
maybeTip = mapExceptT $ fmap $ \case | |||
Left (ErrNetworkTipNetworkUnreachable e) -> Left e | |||
Left (ErrNetworkTipBlockNotFound _) -> Right Nothing | |||
-- HttpBridge never throws ErrNetworkTipBlockNotFound. |
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.
Semi-awkward
Issue Number
#219
Overview
mkNetworkLayer
withnetworkTip
Comments
On
ExceptT ErrNetworkUnreachable m (Maybe Block)
I found it particularly nightmarish to chain
getTipId
andgetBlock
.Therefore I made all
JormungandrLayer
functions returnExceptT ErrNetworkUnreachable
, favouringRight $ Nothing
to indicate 404. f011447NetworkLayer
becomes easier.JormungandrLayer
. I don't see why we can't handle that in theNetworkLayer
instead.Alternative
Edit: the
throwIO
made it complicated, but I now realise that it actually was I who introduced thethrowIO
. The HttpBridge'sthrowIO
(actuallythrowM
) is for a different class of errors.