-
Notifications
You must be signed in to change notification settings - Fork 211
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
Add Jörmungandr Servant Api Type #310
The head ref may contain hidden characters: "anviking/219/j\u00F6rmungandr-network"
Conversation
@Anviking Think about spreading the work over multiple PRs here. You don't have to implement all the endpoint at once. Start with one, and go incrementally 🙏 |
15c4cac
to
8f89736
Compare
0472d86
to
b65b5f4
Compare
data BlockHeader = BlockHeader | ||
{ version :: Word16 | ||
, contentSize :: Word32 | ||
, slot :: SlotId | ||
, chainLength :: Word32 | ||
, contentHash :: Hash "content" | ||
, parentHeaderHash :: Hash "parentHeader" | ||
, parentHeaderHash :: 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.
To match with the one in Cardano.Primitive.Types (BlockHeader)
instance FromBinary Block where | ||
get = getBlock | ||
|
||
instance FromBinary W.Block where |
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.
The Block
type in this module contain more information than the one in Cardano.Wallet.Primitive.Types
(W.Block
).
-- TODO: Replace SignedTx with something real | ||
data SignedTx | ||
|
||
newtype BlockId = BlockId (Hash "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.
Could also have ApiT
, but the jörmungandr api use the term "BlockId" which this newtype can capture.
b65b5f4
to
e92b8e8
Compare
:> "block" | ||
:> Capture "blockId" BlockId | ||
:> QueryParam "count" Int | ||
:> Get '[OctetStream] [BlockId] |
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 am not sure OctetStream
is the best fit here. We actually know the format of the API we're talking to. So it's not just plain bytes. I'd suggest to define some custom format like Jormungandr
or BetterCBOR
or any name that fits to describe the format, and makes it clear that we're still expecting things in a particular format (whereas OctetStream conveys the idea of a stream of bytes without any particular structure).
This will also prevent the orphan 👍
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.
👍
c78975a
to
5793f43
Compare
- Servant type in the .Api module, with Plain and Binary content types. - I added a FromBinary class in the binary module to play nicely with the Binary content-type
5793f43
to
586c722
Compare
Issue Number
#219
Overview
FromBinary
class inCardano.Wallet.Binary.Jörmungandr
Cardano.Wallet.Network.Jörmungandr.Api
where theOctetStream
content type relies on theFromBinary
class.Comments
FromBinary
, orphans, etc