Add Shelley `TransactionLayer` #1673

Anviking · 2020-05-18T12:42:02Z

Issue Number

#1672

Overview

Added toCardano conversion functions interacting with cardano-api
Fixed mistakes in existing conversion functions
Implemented Shelley TransactionLayer and enabled Wallet and Transaction integration tests.
Property test making sure estimateSize never underestimates

Comments

Main commit: 4f6ea17

Anviking · 2020-05-18T12:49:01Z

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

+        -- without converting from @SlotId@.
+        let epochLength = EpochLength 1215
+
+        let Cardano.TxUnsignedShelley unsigned = Cardano.buildShelleyTransaction


Would be nicer to use signTransaction.

But I thought the way cardano-crypto carries an encrypted XPrv and a password to the very end was intentional for… some security reasons? To construct SignKeyDSIGN we'd need to provide the unencrypted key.

was intentional for… some security reasons

Partly. Having key encrypted in-memory was an explicit request from the previous development team which was several times depicted as useless by cryptographers. What matters is storing them encrypted (cf the wallet-chain-libs rust implementation). We still use cardano-crypto for lack of alternative.

Anviking · 2020-05-28T13:45:37Z

We could split this if desired, but I think it should be manageable.

lib/core/src/Cardano/Wallet/Primitive/AddressDerivation/Shelley.hs

KtorZ · 2020-05-28T17:11:08Z

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

+        let certs = []
+
+        -- TODO: The SlotId-SlotNo conversion based on epoch length would not
+        -- work if the epoch length changed in a hard fork.


Although, the transaction itself would be invalid before the hard-fork. True that, subsequent hardfork after shelley may be a risk but for Byron -> Shelley, there's no problem here if we know the new epoch length prior to the hardfork.

era Byron | Shelley el 3 | 2 epoch 0 1 2 3 ========================================= slot 0 |0 3 6 8 V 1 |1 4 7 9 2 |2 5

If we are here given a correct SlotId 3 1 at SlotNo 9 (which arguably would be unlikely, since there's a similar conversion to create the SlotId)

The we cannot simply convert back to SlotNo using the genesis epoch length (here 3):

(SlotNo 3*3+1) == (SlotNo 10) But the correct answer is: SlotNo 9

KtorZ · 2020-05-28T17:12:04Z

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

+
+        -- TODO: The SlotId-SlotNo conversion based on epoch length would not
+        -- work if the epoch length changed in a hard fork.
+        let timeToLive = (toSlotNo epochLength slot) + 7200


7200 ? Avoid magic numbers, make it a named constant and explain the rationale of choosing 7200 (ultimately, we want that configurable via the API). Can you make sure to let Alex know about this?

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

KtorZ · 2020-05-28T17:18:04Z

lib/shelley/src/Cardano/Wallet/Shelley/Transaction/Size.hs

+    -- Maybe that value wasn't taking delegation keys into account?
+    --
+    -- Regardless:
+    -- TODO: Implement properly.


The length of the output will actually depend on the value and the type of addresses used. We can assume worst-case scenario which are currently 65 bytes for addresses indeed. 32 bytes for just the value sounds huge.

KtorZ

Nice work. I think we can go with this first big chunk and do improvements / additions on top.

KtorZ · 2020-05-28T17:20:42Z

bors try

iohk-bors · 2020-05-28T18:19:02Z

try

Build failed

buildkite/cardano-wallet

Useful for playing around

To make it work on shelley, without relying on that the minFee is accurate.

- split functions out so that they read better and will likely be re-used for implementing other parts of the transaction layer. - avoid unnecessary identifiers declaration. Too many identifiers in a single small context don't make things more readable. - changed a few types to avoid repeating the same unwrapping operation - named some in-place calculation (like realFee) - used forM + pattern-matching instead of mapM and pointfree style. Pointfree is usually not readable when it's in a context with more than one line

And tweak calculation

The fee calculation isn't actually made on the transaction size in bytes, but rather, on an abstract representation of the transaction. This is detailed in the ledger specifications and implemented in the cardano-ledger-specs module. So, we might as well re-use this here.

…erated on the fly

By not depending on a pass to the next epoch, which can be quite long on the cardano-node.

KtorZ · 2020-05-30T17:00:39Z

I force pushed the "regenerate nix" commit like a retard. :|

KtorZ · 2020-05-30T17:02:55Z

bors r+

iohk-bors · 2020-05-30T17:31:47Z

Build succeeded

Anviking commented May 18, 2020

View reviewed changes

Anviking self-assigned this May 18, 2020

Anviking force-pushed the anviking/ADP-319/shelley-txs branch from aa5ac1a to 5360a16 Compare May 20, 2020 15:49

KtorZ added the ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG label May 20, 2020

Anviking force-pushed the anviking/ADP-319/shelley-txs branch from 646cd23 to c066371 Compare May 20, 2020 18:36

KtorZ force-pushed the anviking/ADP-319/shelley-txs branch 3 times, most recently from 2183848 to a3df4c1 Compare May 22, 2020 16:29

Anviking force-pushed the anviking/ADP-319/shelley-txs branch 2 times, most recently from 84e9ee4 to 32c2475 Compare May 25, 2020 17:22

paweljakubas mentioned this pull request May 26, 2020

(ADP-257) Add Shelley migration endpoints and Byron (haskell impl) support of migrateWallet #1675

Closed

Anviking force-pushed the anviking/ADP-319/shelley-txs branch 7 times, most recently from 094c617 to 2145ca1 Compare May 28, 2020 13:39

Anviking marked this pull request as ready for review May 28, 2020 13:39

Anviking requested a review from KtorZ May 28, 2020 13:42

KtorZ reviewed May 28, 2020

View reviewed changes

lib/core/src/Cardano/Wallet/Primitive/AddressDerivation/Shelley.hs Outdated Show resolved Hide resolved

KtorZ reviewed May 28, 2020

View reviewed changes

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs Outdated Show resolved Hide resolved

KtorZ reviewed May 28, 2020

View reviewed changes

KtorZ approved these changes May 28, 2020

View reviewed changes

iohk-bors bot added a commit that referenced this pull request May 28, 2020

Try #1673:

b45fd1f

Anviking and others added 20 commits May 30, 2020 18:39

Aux: Add start.sh command for running shelley self-node

c896485

Useful for playing around

Make "TRANS_CREATE_04 - Not enough money" pass more easily

9a82c00

To make it work on shelley, without relying on that the minFee is accurate.

fix weeder

a260fe4

sort dependencies alphabetically in shelley .cabal

df0deb9

Test that shelley tx estimateSize never underestimates

7d307b0

And tweak calculation

Fixup: Use more ada in test to compensate for fee

ca0d78b

Fixup: make jormungandr compile again

eec8612

Explain rationale about the hard-coded TTL value

62c3e17

Fixup: Label network magic NOTEs as FIXMEs

3477db4

implement transaction size estimation & revise existing tests

f965523

use resolved input in integration test fee estimator

90d79f2

oversight: fix temporary directory prefix for shelley launcher

0b7c9f2

increase transaction max size in genesis file

102d3a7

disable some integration tests that aren't possible on Shelley

d6ab901

clean up byron AND shelley wallets too between integration scenario...

aadf637

activate some non-implemented endpoint from the Shelley server

c1c3df5

replace hard-coded (and wrong) address in spec with valid address gen…

0018a9a

…erated on the fly

disable not-yet-covered scenario for migrating big byron wallets

0762a43

KtorZ force-pushed the anviking/ADP-319/shelley-txs branch from bdd431c to 3c1d508 Compare May 30, 2020 16:39

make scenario polling network information more reliable

406bfa2

By not depending on a pass to the next epoch, which can be quite long on the cardano-node.

KtorZ force-pushed the anviking/ADP-319/shelley-txs branch from dbd08da to 406bfa2 Compare May 30, 2020 16:52

Regenerate nix

9eda73b

iohk-bors bot merged commit 3fb783f into master May 30, 2020

iohk-bors bot deleted the anviking/ADP-319/shelley-txs branch May 30, 2020 17:31

Anviking mentioned this pull request Jun 1, 2020

(ADP-319) Add transaction support to cardano-wallet-shelley #1672

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Shelley `TransactionLayer` #1673

Add Shelley `TransactionLayer` #1673

Anviking commented May 18, 2020 •

edited

Loading

Anviking May 18, 2020 •

edited

Loading

KtorZ May 19, 2020 •

edited

Loading

Anviking commented May 28, 2020

KtorZ May 28, 2020

Anviking May 29, 2020

KtorZ May 28, 2020

KtorZ May 28, 2020

KtorZ left a comment

KtorZ commented May 28, 2020

iohk-bors bot commented May 28, 2020

KtorZ commented May 30, 2020

KtorZ commented May 30, 2020

iohk-bors bot commented May 30, 2020

Add Shelley TransactionLayer #1673

Add Shelley TransactionLayer #1673

Conversation

Anviking commented May 18, 2020 • edited Loading

Issue Number

Overview

Comments

Anviking May 18, 2020 • edited Loading

Choose a reason for hiding this comment

KtorZ May 19, 2020 • edited Loading

Choose a reason for hiding this comment

Anviking commented May 28, 2020

KtorZ May 28, 2020

Choose a reason for hiding this comment

Anviking May 29, 2020

Choose a reason for hiding this comment

KtorZ May 28, 2020

Choose a reason for hiding this comment

KtorZ May 28, 2020

Choose a reason for hiding this comment

KtorZ left a comment

Choose a reason for hiding this comment

KtorZ commented May 28, 2020

iohk-bors bot commented May 28, 2020

try

Build failed

KtorZ commented May 30, 2020

KtorZ commented May 30, 2020

iohk-bors bot commented May 30, 2020

Build succeeded

Add Shelley `TransactionLayer` #1673

Add Shelley `TransactionLayer` #1673

Anviking commented May 18, 2020 •

edited

Loading

Anviking May 18, 2020 •

edited

Loading

KtorZ May 19, 2020 •

edited

Loading