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

Implement Jörmungandr NetworkLayer postTx #463

Merged
merged 2 commits into from
Jun 25, 2019
Merged

Conversation

Anviking
Copy link
Member

@Anviking Anviking commented Jun 25, 2019

Issue Number

#219

Overview

  • Renamed JörmungandrLayer postTx to postMessage
  • Implemented JörmungandrLayer postMessage and NetworkLayer postTx
  • Tests
  • TODO: Look over how binary encoders behave / should behave in relation to ErrPostTxBadRequest.

Comments

  • Problem: we can only get details of protocol related errors with jcli rest v0 message logs -h http://127.0.0.1:8081/api. endpoint will silently succeed if
    • Transaction is unbalanced
    • Incorrect signature
    • etc

@@ -140,6 +151,9 @@ data JormungandrLayer m = JormungandrLayer
:: Hash "BlockHeader"
-> Word
-> ExceptT ErrGetDescendants m [Hash "BlockHeader"]
, postMessage
Copy link
Member Author

Choose a reason for hiding this comment

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

  • Avoids collision with NetworkLayer.postTx
  • Coherent with the terminology of the API
  • Signals that we could submit other messages

@Anviking Anviking self-assigned this Jun 25, 2019
@@ -92,7 +94,7 @@ type GetTipId
:> "tip"
:> Get '[Hex] BlockId

type PostSignedTx
type PostMessage
Copy link
Member

Choose a reason for hiding this comment

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

👍

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Api.hs Outdated Show resolved Hide resolved
@@ -92,7 +94,7 @@ type GetTipId
:> "tip"
:> Get '[Hex] BlockId

type PostSignedTx
type PostMessage
= "v0"
:> "message"
:> ReqBody '[JormungandrBinary] (Tx, [TxWitness])
Copy link
Member

Choose a reason for hiding this comment

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

Feels a bit weird though that we do postMesage, but the body has to be a signed transaction... Okay for now though...

-> (Key 'AddressK XPrv, Passphrase "encryption")
-> TxWitness
sign (TxIn (Hash tx) _) (key, (Passphrase pwd)) =
sign (Hash tx) (key, (Passphrase pwd)) =
TxWitness . CC.unXSignature $ CC.sign pwd (getKey key) (block0 <> tx)
Copy link
Member

Choose a reason for hiding this comment

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

Changes to this file included in #464 already 👍

runExceptT (postTx nw signedEmpty) `shouldReturn` Right ()

it "tx from faucet succeeds" $ \(_, nw) -> do
let (Right mw) = mkMnemonic @15 ["tattoo","potato","foil","mutual","slab","path","forward","pencil","suit","marble","hill","meat","close","garden","bird"]
Copy link
Member

Choose a reason for hiding this comment

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

Let go for generateEntropy here instead of a hard-coded mnemonic 👍

Copy link
Member

Choose a reason for hiding this comment

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

Okay, just saw the title now as I was wondering why you'd go for hard-coded addresses in the next tests but not in this one.
We shouldn't hard-coded a generated faucet mnemonic here, there are located in an opaque module for a reason :), they might (and will actually) change over time and they don't have any special semantic so no need to consider them in any particular way.

I think the test would read better if you simply provide some arbitrary inputs and outputs and show that a well-formed transaction can be submitted.

describe "Submitting signed transactions" $ beforeAll startNode' $ afterAll killNode $ do
it "empty tx succeeds" $ \(_, nw) -> do
let signedEmpty = (Tx [] [], [])
runExceptT (postTx nw signedEmpty) `shouldReturn` Right ()
Copy link
Member

Choose a reason for hiding this comment

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

Interesting 🤔

tl
(const $ Just (xprv, mempty))
[ownedIn]
[txOut]
Copy link
Member

Choose a reason for hiding this comment

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

suggestion:

let keystore = const $ Just (xprv, mempty)
let (Right stx) = mkStdTx tl keystore [ownedIn] [txOut]

@Anviking Anviking force-pushed the anviking/219/posttx branch from 4db0977 to bfc3921 Compare June 25, 2019 11:12
@Anviking Anviking marked this pull request as ready for review June 25, 2019 11:14
@Anviking Anviking force-pushed the anviking/219/posttx branch from bfc3921 to 6b3b37f Compare June 25, 2019 11:59
@Anviking Anviking requested a review from KtorZ June 25, 2019 12:14
@Anviking Anviking force-pushed the anviking/219/posttx branch from 6b3b37f to 3c54caa Compare June 25, 2019 12:23
, coin = Coin 933636862791
}
, TxOut
{ address = unsafeDecodeAddress proxy "ca1qwunuat6snw60g99ul6qvte98fjale2k0uu5mrymylqz2ntgzs6vs386wxd"
Copy link
Member

Choose a reason for hiding this comment

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

Careful about line-length here 🙏


it "no input, one output" $ \(_, nw) -> do
let tx = (Tx []
[ (TxOut $ unsafeDecodeAddress proxy "ca1qwunuat6snw60g99ul6qvte98fjale2k0uu5mrymylqz2ntgzs6vs386wxd")
Copy link
Member

Choose a reason for hiding this comment

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

-- this should in practice be like appending bytes to the end of
-- the message.
let signed = (txNonEmpty, [pkWitness, pkWitness, pkWitness])
runExceptT (postTx nw signed) `shouldReturn` Right ()
Copy link
Member

Choose a reason for hiding this comment

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

Interesting as well. Maybe worth reporting to Jörmungandr?

Copy link
Member Author

Choose a reason for hiding this comment

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

@KtorZ KtorZ merged commit 8cc9555 into master Jun 25, 2019
@KtorZ KtorZ deleted the anviking/219/posttx branch June 25, 2019 13:41
@KtorZ KtorZ mentioned this pull request Jul 3, 2019
12 tasks
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.

2 participants