Skip to content
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 golden tests for signing of transactions #203

Merged
merged 6 commits into from
May 2, 2019
Merged

Conversation

Anviking
Copy link
Collaborator

@Anviking Anviking commented Apr 30, 2019

Issue Number

#95

Overview

  • I have added one simple golden test for transaction signing from the cardano-sl implementation
  • I have refactored and fixed the signing code according to this test

Comments

  • This is essentially only one golden test, and not particularly reassuring. Would be nice to generate a couple 100?

@@ -396,7 +396,7 @@ newtype SignedTx = SignedTx { signedTx :: ByteString }
deriving (Show, Eq, Generic)

data TxWitness
= PublicKeyWitness ByteString
= PublicKeyWitness ByteString (Hash "signature")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Key 'AddressK XPub would feel better instead of ByteString, will do in separate pr because of import cycles

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

XPub would probably be sufficient here.

addr `shouldBe` bin


it "decodeSignedTx slBinary == walletTx" $ do
Copy link
Collaborator Author

@Anviking Anviking Apr 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opted to test both using decodeSignedTx and encodeSignedTx. Should add a roundtrip property regardless (I think it doesn't exist. We only test the roundtrip on txs from the BinarySpec blocks.)

bs <- CBOR.decodeBytes
case CBOR.deserialiseFromBytes decodePKWitness (BL.fromStrict bs) of
Left err -> fail $ show err
Right (_, input) -> return input
1 -> CBOR.decodeTag *> (ScriptWitness <$> CBOR.decodeBytes)
2 -> CBOR.decodeTag *> (RedeemWitness <$> CBOR.decodeBytes)
_ -> fail
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I had problems with it previously, I should check that this fail works, and doesn't produce ~infinite output.


fromR = either undefined Prelude.id
in
fromR $ mkStdTx m (error "no root key", pass) [ownedIns] outs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly for what we did for Yoroi golden tests in the AddressDerivationSpec module, it would be better to have a goldenSignedTx utility to generate and compare a transaction binary format from a list of (Address, Key 'AddressK XPub)

Mainnet -> Mainnet
Staging -> Mainnet
Testnet -> Testnet
Local -> Testnet
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I recall correctly, the witness includes the protocol magic too, no ? So all tx should be slightly different even between staging and mainnet. Also, for consistency, we're probably better off with having the same file for local and testnet (not sure there's actually any value checking this for the local network...)

@KtorZ KtorZ force-pushed the anviking/95/SigningSpec branch from 2af4b0b to ddf4939 Compare May 2, 2019 08:47
@KtorZ KtorZ merged commit f54e90f into master May 2, 2019
@KtorZ KtorZ deleted the anviking/95/SigningSpec branch May 2, 2019 09:24
parsonsmatt pushed a commit to parsonsmatt/cardano-sl that referenced this pull request May 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants