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

Test migration of SeaHorses failing due to insufficient ada #2652

Merged
merged 2 commits into from
May 17, 2021

Conversation

Anviking
Copy link
Member

@Anviking Anviking commented May 14, 2021

Issue Number

#2644

Overview

  • Remove pendingWith to enable SHELLEY_CREATE_MIGRATION_PLAN_04 - Cannot create a plan for a wallet that only contains dust.
  • Fund the source wallet with SeaHorses and as small amount of ada to make the migration fail

Comments

@Anviking Anviking requested a review from jonathanknowles May 14, 2021 16:23
@Anviking Anviking marked this pull request as draft May 14, 2021 16:24
@Anviking
Copy link
Member Author

@jonathanknowles I saw the pendingWith in #2644 and got curious if this would work. Feel free to take over, or close if you were to see more problems than value.

@@ -196,18 +197,21 @@ spec = describe "SHELLEY_MIGRATIONS" $ do
(errMsg404NoWallet $ sourceWallet ^. walletId)
]


it "SHELLEY_CREATE_MIGRATION_PLAN_04 - \
\Cannot create a plan for a wallet that only contains dust."
Copy link
Member Author

@Anviking Anviking May 14, 2021

Choose a reason for hiding this comment

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

I guess with this PR this might technically not be a dust wallet?, but it should at least be a wallet of Freeriders

Copy link
Contributor

Choose a reason for hiding this comment

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

For a wallet of just ada values, we could define "dust" as an ada value that can't be an input to a singleton transaction.

Each value would need to be less than output minimum ada quantity + base cost of tx + marginal cost of tx input.

@Anviking
Copy link
Member Author

Also: maybe you don't even need the tokens for a test like this? Just the right amount of ada?

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

@jonathanknowles I saw the pendingWith in #2644 and got curious if this would work. Feel free to take over, or close if you were to see more problems than value.

Hi @Anviking. Thanks for creating this!

I agree with you that it would be ideal to have a test with a real dust wallet, or a wallet that cannot be fully migrated because it's dominated by freerider entries.

An interesting case, perhaps worth testing, would be where the UTxO has too many entries to fit into a single transaction, but the migration algorithm cannot construct a second transaction because all the non-freerider entries are used up in the first transaction. That would allow us to have coverage for a partial migration.

Perhaps we can have a call about it on Monday?

By the way, have a nice weekend!

Base automatically changed from jonathanknowles/multi-asset-migration-server to master May 15, 2021 02:42
@Anviking Anviking force-pushed the anviking/migrate-sea-horses branch from 0d5d0a3 to 1a6eab6 Compare May 17, 2021 11:28
Copy link
Contributor

@jonathanknowles jonathanknowles left a comment

Choose a reason for hiding this comment

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

LGTM!

If you have time, it would be great to add checks that we have a non-zero balance in:

  • SHELLEY_CREATE_MIGRATION_PLAN_04
  • BYRON_CREATE_MIGRATION_PLAN_04

Of course, these are slightly different tests, but they are designed to drive the same condition, which is a non-empty wallet cannot be migrated, even partly.

1. Allow specifying the per-bundle ada quantity when minting SeaHorses
through the test infrastructure.
2. Test that we can get the wallet to 403 when trying to migrate,
despite having a non-zero balance.
@Anviking Anviking force-pushed the anviking/migrate-sea-horses branch from 1a6eab6 to 70bf4f9 Compare May 17, 2021 12:46
@Anviking Anviking self-assigned this May 17, 2021
@Anviking Anviking marked this pull request as ready for review May 17, 2021 12:48
@Anviking
Copy link
Member Author

bors r+

@Anviking
Copy link
Member Author

Anviking commented May 17, 2021

@jonathanknowles I tried adding a similar balance check to the byron version, but it failed 🤔

--- a/lib/core-integration/src/Test/Integration/Scenario/API/Byron/Migrations.hs
+++ b/lib/core-integration/src/Test/Integration/Scenario/API/Byron/Migrations.hs
@@ -202,6 +202,15 @@ spec = describe "BYRON_MIGRATIONS" $ do
             let ep = Link.createMigrationPlan @'Byron sourceWallet
             response <- request @(ApiWalletMigrationPlan n) ctx ep Default
                 (Json [json|{addresses: #{targetAddressIds}}|])
+
+            eventually "dust wallet has funds" $
+                request @ApiWallet ctx
+                    (Link.getWallet @'Shelley sourceWallet) Default Empty
+                    >>= flip verify
+                    [ expectField (#balance . #available . #getQuantity)
+                        (.> 0)
+                    ]
+
             verify response
                 [ expectResponseCode HTTP.status403
                 , expectErrorMessage

🤦‍♂️ -> wait I see why

to BYRON_CREATE_MIGRATION_PLAN_04
@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 17, 2021

Canceled.

@Anviking
Copy link
Member Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 17, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit 2c84173 into master May 17, 2021
@iohk-bors iohk-bors bot deleted the anviking/migrate-sea-horses branch May 17, 2021 20:46
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