-
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
Add initial support for serializing signed transaction #362
Conversation
putWord8 1 | ||
putByteString xPub | ||
putByteString sig | ||
-- TODO: note that we are missing new address type witness: |
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.
we still didn't cover acount and new address witness scheme
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.
Why though?
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.
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.
Got you, though I am bit sad to see that we only merge this while the whole story was expected to be completed already 8 days ago.
-- * https://github.com/input-output-hk/rust-cardano/blob/3524cfe138a10773caa6f0effacf69e792f915df/chain-impl-mockchain/doc/format.md#witnesses | ||
-- * https://github.com/input-output-hk/rust-cardano/blob/e0616f13bebd6b908320bddb1c1502dea0d3305a/chain-impl-mockchain/src/transaction/witness.rs#L23 | ||
ScriptWitness _ -> error "unimplemented: serialize script witness" | ||
RedeemWitness _ -> error "unimplemented: serialize redeem witness" |
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 atm how rust node covers these two cases. Maybe they are encoded under old or normal utxo witness scheme
where | ||
putInput (TxIn inputId inputIx) = do | ||
-- NOTE: special value 0xff indicates account spending | ||
-- only old utxo/address scheme supported for now |
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 have to recall what is new address scheme. I am not sure how to correctly map what we said here https://github.com/input-output-hk/cardano-wallet/blob/master/lib/core/src/Cardano/Wallet/Primitive/Types.hs#L481 with jow rust node encode operates https://github.com/input-output-hk/rust-cardano/blob/3524cfe138a10773caa6f0effacf69e792f915df/chain-impl-mockchain/doc/format.md#token-transfer
Address (bootstrap address 33 bytes, delegation address 65 bytes, account address 33 bytes)
putAddress address | ||
putWord64be $ getCoin coin | ||
putAddress address = do | ||
-- NOTE: only single address supported for now |
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 that we actually have support for other kinds of addresses (old/new/grouped) https://github.com/input-output-hk/cardano-wallet/blob/master/lib/core/src/Cardano/Wallet/Primitive/Types.hs#L481 - so we should be able to implement this I think
Nice, will have a look later. Would it be possible/relevant to generate some golden test cases with |
yes - that's my plan for the rest of the day today |
@akegalj any updates on that one ? |
I am playing with |
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.
@akegalj could we justput
the actual address-bytestring instead of a dummy string, and clean up a bit first? 🙏 Then we can merge this and scaffolding can continue on top. Even without goldens.
:> ReqBody '[JormungandrBinary] SignedTx | ||
:> Post '[NoContent] NoContent | ||
|
||
-- TODO: Replace SignedTx with something real | ||
data SignedTx | ||
type SignedTx = (Tx, [TxWitness]) |
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.
Let's remove this and use ReqBody '[JormungandrBinary] (Tx, [TxWitness])
directly then
@@ -230,6 +272,7 @@ getTokenTransfer = do | |||
let _discrimination = discriminationValue headerByte | |||
case kind of | |||
-- Single Address | |||
-- TODO(akegalj) shouldn't this be `either fail id . decodeAddress (Proxy Jormungandr) <$> getByteString 32` ? |
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.
So - no 👍
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.
yes - removed - forgot to push
-- NOTE: only single address supported for now | ||
putWord8 single | ||
-- TODO(akegalj) this should be `encodeAddress (Proxy Jormungandr) address` | ||
putByteString "addressBytestring" -- address |
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.
So we're essentially just missing a putByteString
of the unwrapped bytes here, and it "should work"?
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.
yes - forgot to push that commit - will push in a sec
e28018d
to
b7a1e14
Compare
b7a1e14
to
ff9e902
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.
LGTM
Issue Number
#219
Overview
Comments