newTransactionLayer for Jormungandr #437
Conversation
|
|
||
| -- | Construct a 'TransactionLayer' compatible with Shelley and 'Jörmungandr' | ||
| newTransactionLayer :: TransactionLayer t | ||
| newTransactionLayer | ||
| :: TransactionLayer (Jormungandr 'Testnet) |
There was a problem hiding this comment.
This shouldn't be fixed to 'TestNet but left as t or, at the very least KnownNetwork n => Jormungandr n
| return (tx, txWitnesses) | ||
|
|
||
| -- NOTE: at this point 'Jörmungandr' node does not support fee calculation | ||
| , estimateSize = \_ -> Quantity 0 |
| -- The wallet only cares about the 'SignTx' tag. In 'cardano-sl' there was | ||
| -- a lot more cases. | ||
| newtype SignTag | ||
| = SignTx (Hash "Tx") |
There was a problem hiding this comment.
I don't think this exists in Jormungandr ?
There was a problem hiding this comment.
yes, signing will be different here, @Anviking will push proper signing. I will concentrate on testing estimateSize in this PR. In the next, I or Johannes will address signing golden tests
There was a problem hiding this comment.
taken signing from Johannes PR as it is
paweljakubas
force-pushed
the
paweljakubas/219/jormungandr-newTransactionLayer
branch
from
b642dfb
to
8bed612
Compare
| instance Arbitrary Address where | ||
| shrink _ = [] | ||
| arbitrary = | ||
| pure $ Address "\131\&3$\195xi\193\"h\154\&5\145}\245:O\"\148\163\165/h^\ENQ\245\248\229;\135\231\234E/" |
There was a problem hiding this comment.
probably will need to be not so specific here, for the test (checking estimatedSize) I added, seems not forbidden
| Hash {getHash = "\216OY\rX\199\234\188.<O\\\244Y\211\210\254\224`i\216\DC3\167\132\139\154\216\161T\174\247\155"} | ||
| tl = newTransactionLayer blockHash :: TransactionLayer (Jormungandr n) | ||
| calcSize = estimateSize tl sel | ||
| in calcSize === Quantity 0 |
There was a problem hiding this comment.
So I take it that we're going to implement the size calculation the transaction layer after all? Or are you planning to leave the quantity at 0? In which case, the whole property + arbitrary instances is maybe overkill don't you think 😅 ?
There was a problem hiding this comment.
yeah, coin selection at this moment should probably be as dummy as possible as estimateSize ignores it. But it will change soon (that is my understanding) and Jormungandr will support fee calculation and they will be valid...
There was a problem hiding this comment.
Generally speaking, it's best to not do things that aren't required on the moment. We have actually no idea when Jormungandr will support fee calculation, although we know that they eventually will.
But, now that it's done, I'd almost be tempted to really implement the size estimator...
| } | ||
| where | ||
| withEither :: e -> Maybe a -> Either e a |
There was a problem hiding this comment.
Very minor - there's functions in both the errors or either packages for this. I find errors pretty useful.
There was a problem hiding this comment.
indeed, in Data.Either.Combinators of either package :
maybeToRight :: b -> Maybe a -> Either b a
Maybe produce a Right, otherwise produce a Left.
>>> maybeToRight "default" (Just 12)
Right 12
>>> maybeToRight "default" Nothing
Left "default"
Issue Number
#219
Overview
Comments