Construct tx for multisig wallet #3378

paweljakubas · 2022-07-07T12:35:59Z

The work was realized in three areas:

On api level, the constructSharedTx was added in core's api level. Also swagger was updated accordingly.
On api's server level the simplest (without delegation or minting support) impl was chosen. The error outlawing calling this endpoint was introduced when shared wallet is in pending state. Also transaction integration test suite was added
The shared wallet state was extended. When the wallet is in active state now both external and internal pools are available. Also PendingIxs was added there. Those three things are now encapsulated in SharedAddressPools abstraction. So the accounting logic is like with shelley's wallet. In order to achieve that SharedAddressPool abstraction was generalized to account for chain type parameter. As previously active state has only external shared address pool, the change to have two more things had significant ripple effects down to DB layer. It also affected core unit testing. As deriveAddressPublicKey from cardano-addresses was assuming external chain only, the prerequisite for this PR is Extend payment share key derivation IntersectMBO/cardano-addresses#191 that enabled also derivation for internal chain.

Moreover, GenChange interface impl was added, also impl of constructAddressFromIx was changed, the same for isShared impl.

constructSharedTransaction was added in Cardano.Wallet. constructTransaction wallet therein could not be used as it sets reward account in a way that is not enough for shared wallets, also it outlawsnon-shelly style wallets. For shared wallets delegation credential could be both keyhash (like for shelley) or script hash - at this moment key hash solution based on accXPub of wallet and ix=0 was added. In next PRs we will return to this subject.

Comments

Issue Number

adp-1623

piotr-iohk · 2022-07-08T14:55:42Z

specifications/api/swagger.yaml

+  /shared-wallets/{walletId}/transactions-construct:
+    post:
+      operationId: constructSharedTransaction
+      tags: ["Transactions New"]


This should go rather into 'Shared Transactions' under 'Shelley (Shared Wallets)'.

piotr-iohk · 2022-07-08T15:03:14Z

specifications/api/swagger.yaml

+      description: |
+        <p align="right">status: <strong>under development</strong></p>
+
+        Create a transaction to be signed from the shared wallet.


I'd be for adding an additional note here explicitely which elements of construct we are expecting to work with a ⚠️. (That's because rendered API doc shows all elements of request body ApiConstructTransactionData which might suggest all of them work)

piotr-iohk · 2022-07-11T13:58:02Z

lib/core-integration/src/Test/Integration/Scenario/API/Shared/Transactions.hs

+        let amt = 10 * minUTxOValue (_mainEra ctx)
+        fundSharedWallet ctx amt walShared
+  where
+     fundSharedWallet ctx amt walShared = do


I think it would be quite useful to introduce fixtureSharedWallet mechanism (just like we have for fixtureWallet). That would be quite handy for testing transactions, as we're going to have more tests like that.

yes, it is good idea. I plan to do it in next PR just not to bloat this one

I have done it in the current PR

piotr-iohk · 2022-07-11T14:09:27Z

lib/core-integration/src/Test/Integration/Framework/TestData.hs

+errMsg403SharedWalletPending :: String
+errMsg403SharedWalletPending = unwords
+    [ "Transaction for a shared wallet should not be tried for"
+    , "a pending shared wallet. Make the wallet active before sending"


The visible status of such wallet is actually:

"state": { "status": "incomplete" }

So maybe something like:

"I cannot construct transaction for a shared wallet that is in 'incomplete' state. Please update your wallet accordingly with 'PATCH /shared-wallets/{walletId}/payment-script-template' or 'PATCH /shared-wallets/{walletId}/delegation-script-template' to make it applicable for constructing transaction."

piotr-iohk · 2022-07-11T15:10:31Z

lib/core-integration/src/Test/Integration/Scenario/API/Shared/Transactions.hs

+            (Link.createUnsignedTransaction @'Shared wal) Default metadata
+        verify rTx
+            [ expectResponseCode HTTP.status403
+            , expectErrorMessage errMsg403EmptyUTxO


Ok for the empty wallet, but we're missing test for constructing tx from wallet with funds.

Actually, it seems the case of non-empty wallet there is an unexpected 403:

$ curl -X POST http://localhost:8090/v2/shared-wallets/e173d884b4300c7450b6a7774fd4055bf3547eac/transactions-construct \ -d '{"payments":[{"address":"addr_test1xz0fh48l3rv4tgs32e767ax60rdncf3ydfgtgxt3u83staue0pjynxnypmrzetew4nmwq4yapd3cc85qc9fg6gva66psrh402e", "amount":{"quantity":10000000,"unit":"lovelace"}}]}' \ -H "Content-Type: application/json" | jq { "code": "invalid_wallet_type", "message": "It is regrettable but you've just attempted an operation that is invalid for this type of wallet. Only new 'Shelley' wallets can do something with rewards and this one isn't." }

I know, I needed to add special mutable account treatment for shared wallet to deal with that.

piotr-iohk · 2022-07-12T16:09:17Z

lib/core-integration/src/Test/Integration/Scenario/API/Shared/Transactions.hs

+        fundSharedWallet ctx amt walShared
+
+        rTx2 <- request @(ApiConstructTransaction n) ctx
+            (Link.createUnsignedTransaction @'Shared wal) Default metadata


It would be nice actually to demonstrate in the integration tests all that we expect to work is working, i.e. constructing:

single output tx

multi ouput tx

multi assets tx

metadata

use of validity intervals

sounds reasonable. I have added testing for all cases except multi assets tx as this topic will require separate treatment and ticket. I also added fixtureSharedWallet as you requested in early comment

HeinrichApfelmus

First round of review. Looks mostly good! 😊 However, a couple of points:

One point that needs to be fixed:

PendingIxs need to be part of the Prologue — similar to how SeqState does it. (Yes, this means that there could be a subtle problem with the pending txs when a rollback happens, they are not part of the checkpoints. However, this problem has always been present in the SeqState; this is not the right moment to fix it.)

Two points that I think should be fixed:

I believe that we can statically set k ~ SharedKey and get rid of the dynamic error for constructSharedTransaction.
Avoid code duplication for PendingIxs, specially for the implementation of nextChangeIndex, by extracting the common functions into a separate module (suggestions in comments).

One point that would be nice to address, but is not blocking:

Accumulate less logic in Cardano.Wallet.Api.Server — move coin selection logic to Cardano.Wallet instead.

Looking forward to second round of review. Happy to discuss in person as well.

(I have looked at almost all files, but not the integration tests.)

lib/shelley/src/Cardano/Wallet/Shelley/Api/Server.hs

lib/core/src/Cardano/Wallet/Api/Server.hs

lib/core/src/Cardano/Wallet.hs

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

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

lib/core/src/Cardano/Wallet/DB/Store/Checkpoints.hs

lib/core/src/Cardano/Wallet/Address/Book.hs

HeinrichApfelmus

Looks good to me now! 😊 Only minor issues with indentation remaining.

HeinrichApfelmus · 2022-07-26T13:13:39Z

lib/core/src/Cardano/Wallet/DB/Store/Checkpoints.hs

+              Shared.pendingIxsFromList . fromRes <$> selectList
+              [SharedStatePendingWalletId ==. wid]
+              [Desc SharedStatePendingIxIndex]


Suggested change

Shared.pendingIxsFromList . fromRes <$> selectList

[SharedStatePendingWalletId ==. wid]

[Desc SharedStatePendingIxIndex]

Shared.pendingIxsFromList . fromRes <$> selectList

[SharedStatePendingWalletId ==. wid]

[Desc SharedStatePendingIxIndex]

(Indentation for clarity.)

HeinrichApfelmus · 2022-07-26T13:14:30Z

lib/core/src/Cardano/Wallet/DB/Store/Checkpoints.hs

+        extAddrMap <- loadAddresses @'UtxoExternal
+        intAddrMap <- loadAddresses @'UtxoInternal
+        pure $ SharedDiscoveries extAddrMap intAddrMap


Suggested change

extAddrMap <- loadAddresses @'UtxoExternal

intAddrMap <- loadAddresses @'UtxoInternal

pure $ SharedDiscoveries extAddrMap intAddrMap

extAddrMap <- loadAddresses @'UtxoExternal

intAddrMap <- loadAddresses @'UtxoInternal

pure $ SharedDiscoveries extAddrMap intAddrMap

HeinrichApfelmus · 2022-07-26T13:15:37Z

lib/core/src/Cardano/Wallet/Primitive/AddressDiscovery/Shared.hs

+-------------------------------------------------------------------------------}
+
+-- | An ordered set of pending indexes. This keep track of indexes used
+newtype PendingIxs = PendingIxs


let's say I will do it in the next PR

That works for me! Feel free to make a separate PR just for this refactoring move — that will make it much easier to review.

paweljakubas · 2022-07-26T13:55:54Z

bors r+

3378: Construct tx for multisig wallet r=paweljakubas a=paweljakubas  The work was realized in three areas: 1. On api level, the constructSharedTx was added in core's api level. Also swagger was updated accordingly. On api's server level the simplest (without delegation or minting support) impl was chosen. The error outlawing calling this endpoint was introduced when shared wallet is in pending state. Also transaction integration test suite was added 2. The shared wallet state was extended. When the wallet is in active state now both external and internal pools are available. Also `PendingIxs` was added there. Those three things are now encapsulated in `SharedAddressPools` abstraction. So the accounting logic is like with shelley's wallet. In order to achieve that `SharedAddressPool` abstraction was generalized to account for chain type parameter. As previously active state has only external shared address pool, the change to have two more things had significant ripple effects down to DB layer. It also affected core unit testing. As `deriveAddressPublicKey` from cardano-addresses was assuming external chain only, the prerequisite for this PR is IntersectMBO/cardano-addresses#191 that enabled also derivation for internal chain. Moreover, `GenChange` interface impl was added, also impl of `constructAddressFromIx` was changed, the same for `isShared` impl. 3. `constructSharedTransaction` was added in `Cardano.Wallet`. `constructTransaction` wallet therein could not be used as it sets reward account in a way that is not enough for shared wallets, also it outlawsnon-shelly style wallets. For shared wallets delegation credential could be both keyhash (like for shelley) or script hash - at this moment key hash solution based on accXPub of wallet and ix=0 was added. In next PRs we will return to this subject. ### Comments  ### Issue Number adp-1623  Co-authored-by: Pawel Jakubas <[email protected]> Co-authored-by: Piotr Stachyra <[email protected]>

iohk-bors · 2022-07-26T14:52:57Z

Build failed:

ci/hydra-build:required

  src/Test/Integration/Scenario/API/Shared/Addresses.hs:86:9: 
  1) API Specifications, SHARED_ADDRESSES, SHARED_ADDRESSES_LIST_01 - Can list known addresses on a default wallet
       expected: 20
        but got: 40

paweljakubas · 2022-07-26T15:26:17Z

bors r+

iohk-bors · 2022-07-26T16:34:32Z

Build succeeded:

3412: PendingIxs refactoring and decode tx shared wallets r=paweljakubas a=paweljakubas  - [x] I have introduced `PendingIxs` in `AddressDerivation` and reused it in both sequential and shared mode - [x] I have implemented endpoint for decode tx for multisig working for all cases supported for construct tx as in #3378 - [x] I have added integration tests ### Comments  ### Issue Number adp-2008  Co-authored-by: Pawel Jakubas <[email protected]>

paweljakubas self-assigned this Jul 7, 2022

paweljakubas force-pushed the paweljakubas/adp-1623/construct-decode-tx-multisig branch from 6b61367 to f026f64 Compare July 7, 2022 15:45

paweljakubas requested a review from jonathanknowles July 7, 2022 16:05

paweljakubas force-pushed the paweljakubas/adp-1623/construct-decode-tx-multisig branch from f026f64 to e476ff6 Compare July 8, 2022 14:05

paweljakubas marked this pull request as ready for review July 8, 2022 14:05

piotr-iohk reviewed Jul 8, 2022

View reviewed changes

piotr-iohk reviewed Jul 11, 2022

View reviewed changes

paweljakubas force-pushed the paweljakubas/adp-1623/construct-decode-tx-multisig branch from dfde761 to 6892586 Compare July 12, 2022 13:53

paweljakubas changed the title ~~Construct tx for multisig multisig~~ Construct tx for multisig wallet Jul 12, 2022

paweljakubas requested a review from piotr-iohk July 12, 2022 14:20

piotr-iohk reviewed Jul 12, 2022

View reviewed changes

paweljakubas force-pushed the paweljakubas/adp-1623/construct-decode-tx-multisig branch from 16aa0d6 to 9532b60 Compare July 14, 2022 14:07

paweljakubas requested a review from piotr-iohk July 14, 2022 14:10

paweljakubas mentioned this pull request Jul 19, 2022

Decode tx for multisig #3400

Closed

2 tasks

HeinrichApfelmus reviewed Jul 22, 2022

View reviewed changes

paweljakubas force-pushed the paweljakubas/adp-1623/construct-decode-tx-multisig branch from 9532b60 to 1ce7652 Compare July 26, 2022 12:27

paweljakubas requested a review from HeinrichApfelmus July 26, 2022 12:27

paweljakubas added 12 commits July 26, 2022 14:29

more work at api level

2e522cf

extract and reuse handleValidityInterval

e1c1900

add missing instances for SharedKey and SharedState - part 1

063b902

rethink GenChange for SharedState

97bb7d2

fix core unit tests by adding swagger entry

2b5f9f4

introduce SharedAddressPools

596f739

adjust book and db layer for SharedAddressPools

0d73a3c

parametrize address construction for shared wallets

2dd22ef

bump cardano-addresses temporarily

876170e

adjust code to make all core unit tests passing

894952c

impl genChange interface

5ba2122

bump to the cardano-addresses

75d68e3

paweljakubas and others added 13 commits July 26, 2022 14:29

add pending error plus tx test suite

852baf6

first integration test working

7d3f164

add active shared wallet integration tests - part 1

a3f83c8

add active shared wallet integration tests - part 2

2ce81ab

swagger.yaml update for improved rendering

b0f66e4

deal with reward error for shared wallets

3e9728b

more improvements

9e59b5c

add fixtureSharedWallet

858a46a

add more integration testing

e396609

review part 1

2254f0b

review part 2

72fda04

review part 3

1ce7652

hlint

d01335e

HeinrichApfelmus approved these changes Jul 26, 2022

View reviewed changes

more cleaning

8c13f3d

update shared address integration test

8ab82fe

iohk-bors bot merged commit ea35080 into master Jul 26, 2022

iohk-bors bot deleted the paweljakubas/adp-1623/construct-decode-tx-multisig branch July 26, 2022 16:34

WilliamKingNoel-Bot pushed a commit that referenced this pull request Jul 26, 2022

docs: Merge #3378

a83b7d6

paweljakubas mentioned this pull request Jul 28, 2022

PendingIxs refactoring and decode tx shared wallets #3412

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Construct tx for multisig wallet #3378

Construct tx for multisig wallet #3378

paweljakubas commented Jul 7, 2022 •

edited

Loading

piotr-iohk Jul 8, 2022 •

edited

Loading

paweljakubas Jul 8, 2022

piotr-iohk Jul 8, 2022

paweljakubas Jul 8, 2022

piotr-iohk Jul 11, 2022

paweljakubas Jul 12, 2022

paweljakubas Jul 26, 2022

piotr-iohk Jul 11, 2022 •

edited

Loading

paweljakubas Jul 12, 2022

piotr-iohk Jul 11, 2022

paweljakubas Jul 12, 2022 •

edited

Loading

piotr-iohk Jul 12, 2022

paweljakubas Jul 14, 2022

HeinrichApfelmus left a comment

HeinrichApfelmus left a comment

HeinrichApfelmus Jul 26, 2022

paweljakubas Jul 26, 2022

HeinrichApfelmus Jul 26, 2022

paweljakubas Jul 26, 2022

HeinrichApfelmus Jul 26, 2022

paweljakubas commented Jul 26, 2022

iohk-bors bot commented Jul 26, 2022 •

edited by Anviking

Loading

paweljakubas commented Jul 26, 2022

iohk-bors bot commented Jul 26, 2022

Construct tx for multisig wallet #3378

Construct tx for multisig wallet #3378

Conversation

paweljakubas commented Jul 7, 2022 • edited Loading

Comments

Issue Number

piotr-iohk Jul 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piotr-iohk Jul 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas Jul 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HeinrichApfelmus left a comment

Choose a reason for hiding this comment

HeinrichApfelmus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas commented Jul 26, 2022

iohk-bors bot commented Jul 26, 2022 • edited by Anviking Loading

paweljakubas commented Jul 26, 2022

iohk-bors bot commented Jul 26, 2022

paweljakubas commented Jul 7, 2022 •

edited

Loading

piotr-iohk Jul 8, 2022 •

edited

Loading

piotr-iohk Jul 11, 2022 •

edited

Loading

paweljakubas Jul 12, 2022 •

edited

Loading

iohk-bors bot commented Jul 26, 2022 •

edited by Anviking

Loading