Connect API with migration algorithm #2644

jonathanknowles · 2021-05-10T08:03:08Z

Issue Number

Overview

This PR:

Implements Api.Server.createWalletMigrationPlan.
Implements Api.Server.migrateWallet.
Adjusts migrateWallet to require a non-empty list of addresses.
Adjusts the success response types for all migration endpoints to be 202 ACCEPTED.
Resurrects all previously-disabled integration tests (for ada-specific wallets).
Removes the "disabled" warnings on migration endpoints.
Adds integration test coverage for MA wallets.

QA Due Diligence

After rewriting the integration test suite, I ran the entire integration test suite 500 times in an effort to increase confidence that there are no flaky tests. The test suite passed 500 times, with no failures or errors.

Anviking

Will look closer later, but looks good 👍

Anviking · 2021-05-11T21:52:48Z

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/Migrations.hs

@@ -193,7 +220,7 @@ spec = describe "SHELLEY_MIGRATIONS" $ do
              testAddressCycling 3
              testAddressCycling 10

-    Hspec.it "SHELLEY_MIGRATE_01_big_wallet - \


🙅‍♂️ I would not. Hspec.it doesn't retry on failures. it does retry, but the funds of the hard-coded mnemonic can only be spent once, most likely making it pointless.

hoping we can remove the retrying it soon

That's interesting! I had no idea that this difference even existed. Thanks for pointing it out.

I'm just thinking how we could make this difference clearer, so that the name of the function makes it obvious whether or not the contents will be retried on failure.

Fixed in d4f6fd6.

rvl

This seems pretty good so far 👍. You could also update the migration endpoint status info in swagger.yaml. Unnecessary whitespace changes make it harder to provide a good review.

lib/core/src/Cardano/Wallet.hs

rvl · 2021-05-12T05:53:12Z

lib/core/src/Cardano/Wallet.hs

@@ -2310,6 +2413,11 @@ data ErrStartTimeLaterThanEndTime = ErrStartTimeLaterThanEndTime
    , errEndTime :: UTCTime
    } deriving (Show, Eq)

+data ErrCreateMigrationPlan
+    = ErrCreateMigrationPlanEmpty


Couldn't it just return and empty list of transactions? That seems fairly obvious and easy to handle for users.

You're right, it could.

However, the NothingToMigrate condition was already a specific error in the old migrateWallet API. This PR just reuses the same approach for createMigrationPlan.

It covers two cases:

The wallet is genuinely empty (therefore there really is nothing available to migrate).

The wallet isn't empty, but the migration algorithm wasn't able to pay for any of the UTxO entries to be migrated, and therefore the plan is empty.

For the second case, I think it's useful to have a specific error here, as we can provide a useful hint to the user:

I wasn't able to construct a migration plan. This could be because your wallet is empty, or it could be because the amount of ada in your wallet is insufficient to pay for any of the funds to be migrated. Try adding some ada to your wallet before trying again."

For comparison, the original message was:

I can't migrate the wallet with the given ID, because it's either empty or full of small coins which wouldn't be worth migrating."

Arguably it might want to be worth including the case-specific balanceLeftover in the 403 case.

Could we have a err403 but returning ApiWalletMigrationPlan with selections = []? It couldn't include an error message though, and perhaps a bit weird, so probably not.

Could we have a err403 but returning ApiWalletMigrationPlan with selections = []? It couldn't include an error message though, and perhaps a bit weird, so probably not.

In that specific case, the balanceLeftover would be precisely the same as the wallet's balance. So perhaps it wouldn't be providing any more info than the user already has. (Presumably, they already know their balance, or can easily check it.)

rvl · 2021-05-12T05:53:12Z

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

+    -> MigrationPlan
+    -> Maybe (ApiWalletMigrationPlan n)
+mkApiWalletMigrationPlan s addresses rewardWithdrawal plan
+    | Just selections <- maybeSelections =


case maybeSelections of

Ah yes, the pattern guard is a bit unnecessary here!

(I think it might be a hangover from a previous version of the function.)

I've fixed this in a671d6e.

rvl · 2021-05-12T05:54:11Z

lib/core-integration/src/Test/Integration/Scenario/API/Byron/Migrations.hs

+            [ expectResponseCode HTTP.status202
+            , expectField
+                (#totalFee . #getQuantity)
+                (`shouldBe` 2_460_400)


In integration tests, exact values like this may cause us pain in future.
It might be possible to make the test case more forgiving of changes, without reducing its effectiveness.

Hi @rvl. I think I understand your concerns here.

However, I think there are good arguments in favour of using exact values:

The new migration algorithm is completely deterministic: the plan is fully determined by the current UTxO, with no randomness. So if we start with the same fixture wallet and transaction constraints, we should always end up with precisely the same plan.

To increase confidence that there are no flaky tests, I've run the entire migration integration test suite 500 times and confirmed that it always passes.

Stating exact values means we can increase our confidence that the API is producing plans with reasonable fees, since fees are explicitly encoded as literals in the test code. I think it's good to be a bit paranoid here, as the potential cost to a user of miscalculating a fee is that they spend more ada than required (potentially without noticing).

If someone alters the fixture wallet definitions or the fee calculation algorithm, then it will cause the tests to fail. However, they're likely to fail in an extremely obvious way, and it's likely that we can fix them very quickly just by entering new numbers. It also gives a fresh chance for a human pair of eyes to confirm that fees produced by the API are reasonable.

If such a test fails when run the first time, it's more than likely that it'll fail when run a second time (as a fixture wallet's funds can only be spent once). In response to review feedback: #2644 (comment)

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

If such a test fails when run the first time, it's more than likely that it'll fail when run a second time (as a fixture wallet's funds can only be spent once). In response to review feedback: #2644 (comment)

In response to review feedback: #2644 (comment)

jonathanknowles · 2021-05-13T10:42:06Z

bors try

iohk-bors · 2021-05-13T12:11:37Z

try

Build succeeded:

Anviking

👍

Anviking · 2021-05-14T10:25:39Z

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/Migrations.hs

+                    (`shouldBe` 255_200)
+                , expectField (#selections)
+                    ((`shouldBe` 1) . length)
+                , expectField (#balanceSelected . #ada . #getQuantity)
+                    (`shouldBe` 1_000_000_000_000)
+                , expectField (#balanceLeftover . #ada . #getQuantity)
+                    (`shouldBe` 0)


Anviking · 2021-05-14T15:49:51Z

lib/core/src/Cardano/Wallet.hs

@@ -17,6 +17,7 @@
 {-# LANGUAGE TypeApplications #-}
 {-# LANGUAGE TypeFamilies #-}
 {-# LANGUAGE TypeOperators #-}
+{-# OPTIONS_GHC -fno-warn-redundant-constraints #-}


☝️ Guessing your HLS quick-fix added this by mistake

Or it was intentional?

Could we get by with a type alias instead, not to have to disable the warning for the entire module?

@Anviking Thanks, this is a good suggestion. 👍🏻

I also had reservations about disabling this warning. It's a shame that it's not possible to disable these warnings for just a single function. (Or is it?) I notice that we disable this warning in other modules -- perhaps we shouldn't do that.

I agree with you that it's just simpler to use a type synonym here.

I've fixed this in b913667.

Anviking · 2021-05-14T16:02:52Z

lib/core/src/Cardano/Wallet.hs

@@ -2310,6 +2413,11 @@ data ErrStartTimeLaterThanEndTime = ErrStartTimeLaterThanEndTime
    , errEndTime :: UTCTime
    } deriving (Show, Eq)

+data ErrCreateMigrationPlan
+    = ErrCreateMigrationPlanEmpty


Arguably it might want to be worth including the case-specific balanceLeftover in the 403 case.

Could we have a err403 but returning ApiWalletMigrationPlan with selections = []? It couldn't include an error message though, and perhaps a bit weird, so probably not.

Supplying an empty list of addresses would make it impossible to perform a migration.

This check is actually already performed in other integration tests. In addition, we can easily add this check to the end of tests that don't have it already.

If such a test fails when run the first time, it's more than likely that it'll fail when run a second time (as a fixture wallet's funds can only be spent once). In response to review feedback: #2644 (comment)

In response to review feedback: #2644 (comment)

This allows us to remove the module-wide `fno-warn-redundant-constraints`. In response to review feedback: #2644 (comment)

jonathanknowles · 2021-05-14T23:59:00Z

bors r+

2644: Connect API with migration algorithm r=jonathanknowles a=jonathanknowles # Issue Number ADP-840 # Overview This PR: - [x] Implements `Api.Server.createWalletMigrationPlan`. - [x] Implements `Api.Server.migrateWallet`. - [x] Adjusts `migrateWallet` to require a non-empty list of addresses. - [x] Adjusts the success response types for all migration endpoints to be `202` `ACCEPTED`. - [x] Resurrects all previously-disabled integration tests (for ada-specific wallets). - [x] Removes the "disabled" warnings on migration endpoints. - [x] Adds integration test coverage for MA wallets. # QA Due Diligence After rewriting the integration test suite, I ran the entire integration test suite 500 times in an effort to increase confidence that there are no flaky tests. The test suite passed 500 times, with no failures or errors. Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io>

iohk-bors · 2021-05-15T00:26:46Z

Build failed:

ci/hydra-build:required

Failure:

Failures:

  test/unit/Network/Wai/Middleware/LoggingSpec.hs:118:9:
  1) Network.Wai.Middleware.Logging, Logging Middleware, GET, 200, no query
       uncaught exception: HttpException
       HttpExceptionRequest Request {
         host                 = "localhost"
         port                 = 63839
         secure               = False
         requestHeaders       = []
         path                 = "/get"
         queryString          = ""
         method               = "GET"
         proxy                = Nothing
         rawBody              = False
         redirectCount        = 10
         responseTimeout      = ResponseTimeoutDefault
         requestVersion       = HTTP/1.1
       }
        ConnectionTimeout

  To rerun use: --match "/Network.Wai.Middleware.Logging/Logging Middleware/GET, 200, no query/"

Randomized with seed 1332915072

#2217

jonathanknowles · 2021-05-15T00:36:48Z

bors r+

2644: Connect API with migration algorithm r=jonathanknowles a=jonathanknowles # Issue Number ADP-840 # Overview This PR: - [x] Implements `Api.Server.createWalletMigrationPlan`. - [x] Implements `Api.Server.migrateWallet`. - [x] Adjusts `migrateWallet` to require a non-empty list of addresses. - [x] Adjusts the success response types for all migration endpoints to be `202` `ACCEPTED`. - [x] Resurrects all previously-disabled integration tests (for ada-specific wallets). - [x] Removes the "disabled" warnings on migration endpoints. - [x] Adds integration test coverage for MA wallets. # QA Due Diligence After rewriting the integration test suite, I ran the entire integration test suite 500 times in an effort to increase confidence that there are no flaky tests. The test suite passed 500 times, with no failures or errors. Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io>

iohk-bors · 2021-05-15T01:32:41Z

Build failed:

buildkite/cardano-wallet

Failure:

�[0mcardano-wallet             >   src/Test/Integration/Scenario/CLI/Shelley/Wallets.hs:742:18: �[0m
�[0mcardano-wallet             >   1) CLI Specifications, SHELLEY_CLI_WALLETS, WALLETS_UTXO_02 - Utxo statistics works properly�[0m
�[0mcardano-wallet             >        Waited longer than 90s to resolve action: "Wallet balance is as expected".�[0m
�[0mcardano-wallet             > �[33m[cardano-wallet.network:Warning:54677]�[0m [2021-05-15 01:30:49.72 UTC] Connection lost with the node. Network.Socket.recvBuf: resource vanished (Connection reset by peer)�[0m
�[0mcardano-wallet             > �[33m[cardano-wallet.network:Warning:53388]�[0m [2021-05-15 01:30:49.72 UTC] Connection lost with the node. Network.Socket.recvBuf: resource vanished (Connection reset by peer)�[0m
�[0mcardano-wallet             > �[33m[cardano-wallet.network:Warning:2060]�[0m [2021-05-15 01:30:49.72 UTC] Connection lost with the node. writev: resource vanished (Broken pipe)�[0m
�[0mcardano-wallet             > �[33m[cardano-wallet.network:Warning:52413]�[0m [2021-05-15 01:30:49.72 UTC] Connection lost with the node. Network.Socket.recvBuf: resource vanished (Connection reset by peer)�[0m
�[0mcardano-wallet             > �[33m[cardano-wallet.network:Warning:51774]�[0m [2021-05-15 01:30:49.73 UTC] Connection lost with the node. Network.Socket.recvBuf: resource vanished (Connection reset by peer)�[0m
�[0mcardano-wallet             >        expected: Quantity {getQuantity = 1562000000}�[0m
�[0mcardano-wallet             >         but got: Quantity {getQuantity = 0}�[0m
�[0mcardano-wallet             > �[0m
�[0mcardano-wallet             >   To rerun use: --match "/CLI Specifications/SHELLEY_CLI_WALLETS/WALLETS_UTXO_02 - Utxo statistics works properly/"�[0m
�[0mcardano-wallet             > �[0m
�[0mcardano-wallet             >   src/Test/Integration/Scenario/CLI/Shelley/HWWallets.hs:129:18: �[0m

This looks superficially similar to the following issue, but it seems as though the underlying cause is the node disappearing. (Perhaps that issue also has the same underlying cause.)

#2429

jonathanknowles · 2021-05-15T01:38:59Z

bors r+

iohk-bors · 2021-05-15T02:42:28Z

Build succeeded:

2644: Connect API with migration algorithm r=jonathanknowles a=jonathanknowles # Issue Number ADP-840 # Overview This PR: - [x] Implements `Api.Server.createWalletMigrationPlan`. - [x] Implements `Api.Server.migrateWallet`. - [x] Adjusts `migrateWallet` to require a non-empty list of addresses. - [x] Adjusts the success response types for all migration endpoints to be `202` `ACCEPTED`. - [x] Resurrects all previously-disabled integration tests (for ada-specific wallets). - [x] Removes the "disabled" warnings on migration endpoints. - [x] Adds integration test coverage for MA wallets. # QA Due Diligence After rewriting the integration test suite, I ran the entire integration test suite 500 times in an effort to increase confidence that there are no flaky tests. The test suite passed 500 times, with no failures or errors. Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io> a60199f

2651: Testing shared wallets r=piotr-iohk a=piotr-iohk # Issue Number ADP-866 # Overview - 73225b4 fix e2e tests to comply with purpose=1854' for multisig keys - c475917 Tests for cosigner self - ed846f0 Tests for public keys - 40698f4 Tests for Account Public Keys # Comments 2652: Test migration of SeaHorses failing due to insufficient ada r=Anviking a=Anviking # Issue Number  #2644 # Overview  - [x] Remove `pendingWith` to enable `SHELLEY_CREATE_MIGRATION_PLAN_04 - Cannot create a plan for a wallet that only contains dust.` - [x] Fund the source wallet with SeaHorses and as small amount of ada to make the migration fail # Comments   Co-authored-by: Piotr Stachyra <piotr.stachyra@iohk.io> Co-authored-by: Johannes Lund <johannes.lund@iohk.io>

jonathanknowles force-pushed the jonathanknowles/multi-asset-migration-server branch from c651d40 to f3c48e8 Compare May 10, 2021 08:03

jonathanknowles requested a review from Anviking May 10, 2021 08:04

jonathanknowles changed the title ~~WIP: Implement Migration API~~ Implement Migration API May 10, 2021

jonathanknowles changed the title ~~Implement Migration API~~ Connect API with migration algorithm May 10, 2021

jonathanknowles force-pushed the jonathanknowles/multi-asset-migration-server branch from f3c48e8 to 5082eb6 Compare May 11, 2021 09:03

Anviking reviewed May 11, 2021

View reviewed changes

jonathanknowles force-pushed the jonathanknowles/multi-asset-migration-server branch from 5082eb6 to ababf43 Compare May 12, 2021 04:39

rvl reviewed May 12, 2021

View reviewed changes

jonathanknowles force-pushed the jonathanknowles/multi-asset-migration-server branch 2 times, most recently from 5111544 to d6771ce Compare May 13, 2021 06:25

jonathanknowles force-pushed the jonathanknowles/multi-asset-migration-server branch from 7ada448 to aef4f52 Compare May 13, 2021 07:54

jonathanknowles commented May 13, 2021

View reviewed changes

lib/core/src/Cardano/Wallet/Api/Server.hs Outdated Show resolved Hide resolved

jonathanknowles added a commit that referenced this pull request May 13, 2021

Use more idiomatic style in mkApiWalletMigrationPlan.

a671d6e

In response to review feedback: #2644 (comment)

jonathanknowles force-pushed the jonathanknowles/multi-asset-migration-server branch from aef4f52 to a671d6e Compare May 13, 2021 08:46

jonathanknowles added a commit that referenced this pull request May 13, 2021

Provide a useful error message when there's nothing to migrate.

713990d

In response to review feedback: #2644 (comment)

iohk-bors bot added a commit that referenced this pull request May 13, 2021

Try #2644:

1498290

jonathanknowles self-assigned this May 13, 2021

jonathanknowles marked this pull request as ready for review May 14, 2021 09:28

jonathanknowles requested a review from Anviking May 14, 2021 09:28

Anviking approved these changes May 14, 2021

View reviewed changes

Anviking mentioned this pull request May 14, 2021

Test migration of SeaHorses failing due to insufficient ada #2652

Merged

2 tasks

jonathanknowles added 3 commits May 14, 2021 23:56

Make ApiWalletMigrationPostData.addresses non-empty.

fc676e4

Supplying an empty list of addresses would make it impossible to perform a migration.

Add function Wallet.migrationPlanToUnsignedTxs.

e295ca4

Add function Wallet.createMigrationPlan.

0d9493f

jonathanknowles added 17 commits May 14, 2021 23:57

Resurrect test CREATE_MIGRATION_PLAN_04.

267339d

Resurrect test CREATE_MIGRATION_PLAN_05.

65cbe79

Resurrect test MIGRATE_01.

476edd1

Resurrect test MIGRATE_02.

b50f6a8

Resurrect test MIGRATE_03.

00f4120

Resurrect test MIGRATE_04.

b1c718c

Resurrect test MIGRATE_05.

1fbf614

Resurrect test MIGRATE_06.

c220abb

Resurrect test MIGRATE_07.

2f9723e

Remove migration test that checks for depletion of source funds.

d6d2abc

This check is actually already performed in other integration tests. In addition, we can easily add this check to the end of tests that don't have it already.

Resurrect test MIGRATE_08.

33e0562

Don't retry tests that deplete a fixture wallet's funds.

5aabaa1

If such a test fails when run the first time, it's more than likely that it'll fail when run a second time (as a fixture wallet's funds can only be spent once). In response to review feedback: #2644 (comment)

Use more idiomatic style in mkApiWalletMigrationPlan.

eb96557

In response to review feedback: #2644 (comment)

Provide a useful error message when there's nothing to migrate.

a3d7dee

In response to review feedback: #2644 (comment)

Remove "disabled" warnings from migration endpoints.

b0f1d82

Add test MIGRATE_MULTI_ASSET_01.

281998f

Use type synonym for selection results without change.

b913667

This allows us to remove the module-wide `fno-warn-redundant-constraints`. In response to review feedback: #2644 (comment)

jonathanknowles force-pushed the jonathanknowles/multi-asset-migration-server branch from 2069746 to b913667 Compare May 14, 2021 23:57

iohk-bors bot merged commit a60199f into master May 15, 2021

iohk-bors bot deleted the jonathanknowles/multi-asset-migration-server branch May 15, 2021 02:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Connect API with migration algorithm #2644

Connect API with migration algorithm #2644

jonathanknowles commented May 10, 2021 •

edited

Loading

Anviking left a comment

Anviking May 11, 2021

Anviking May 11, 2021

jonathanknowles May 12, 2021

jonathanknowles May 13, 2021 •

edited

Loading

rvl left a comment

rvl May 12, 2021

jonathanknowles May 13, 2021 •

edited

Loading

Anviking May 14, 2021

jonathanknowles May 15, 2021

rvl May 12, 2021

jonathanknowles May 13, 2021

rvl May 12, 2021

jonathanknowles May 14, 2021 •

edited

Loading

jonathanknowles commented May 13, 2021

iohk-bors bot commented May 13, 2021

Anviking left a comment

Anviking May 14, 2021

Anviking May 14, 2021

Anviking May 14, 2021

jonathanknowles May 14, 2021 •

edited

Loading

Anviking May 14, 2021

jonathanknowles commented May 14, 2021

iohk-bors bot commented May 15, 2021 •

edited by jonathanknowles

Loading

jonathanknowles commented May 15, 2021

iohk-bors bot commented May 15, 2021 •

edited by jonathanknowles

Loading

jonathanknowles commented May 15, 2021

iohk-bors bot commented May 15, 2021

Connect API with migration algorithm #2644

Connect API with migration algorithm #2644

Conversation

jonathanknowles commented May 10, 2021 • edited Loading

Issue Number

Overview

QA Due Diligence

Anviking 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

jonathanknowles May 13, 2021 • edited Loading

Choose a reason for hiding this comment

rvl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanknowles May 13, 2021 • 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

jonathanknowles May 14, 2021 • edited Loading

Choose a reason for hiding this comment

jonathanknowles commented May 13, 2021

iohk-bors bot commented May 13, 2021

try

Anviking 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

jonathanknowles May 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanknowles commented May 14, 2021

iohk-bors bot commented May 15, 2021 • edited by jonathanknowles Loading

jonathanknowles commented May 15, 2021

iohk-bors bot commented May 15, 2021 • edited by jonathanknowles Loading

jonathanknowles commented May 15, 2021

iohk-bors bot commented May 15, 2021

jonathanknowles commented May 10, 2021 •

edited

Loading

jonathanknowles May 13, 2021 •

edited

Loading

jonathanknowles May 13, 2021 •

edited

Loading

jonathanknowles May 14, 2021 •

edited

Loading

jonathanknowles May 14, 2021 •

edited

Loading

iohk-bors bot commented May 15, 2021 •

edited by jonathanknowles

Loading

iohk-bors bot commented May 15, 2021 •

edited by jonathanknowles

Loading