Test that `migrateByronWallet` migrates funds correctly. #950

jonathanknowles · 2019-11-04T06:30:49Z

Issue Number

Overview

This PR tests that after a successful call to migrateByronWallet:

all funds are debited from the source wallet, leaving a balance of 0.
all funds are eventually credited to the target wallet, minus the fee.
the resultant fee is the same as the fee predicted by getByronWalletMigrateInfo.

KtorZ · 2019-11-05T08:38:05Z

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

+            verify r0
+                [ expectResponseCode @IO HTTP.status200
+                , expectFieldSatisfy balanceAvailable (> 0)
+                ]


This check is superfluous, that is what fixture*Wallet* gives you already. The function only returns when funds are available.

This check is superfluous, that is what fixture*Wallet* gives you already. The function only returns when funds are available.

Fair enough. I added this as a sanity check (and to make the assumptions more explicit), but we can remove it if we're confident that the fixtureByronWallet will only ever return a wallet with non-zero funds.

KtorZ · 2019-11-05T08:38:41Z

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

+                (Json [aesonQQ|{"passphrase": #{sourceWalletPass}}|])
+            verify r1
+                [ expectResponseCode @IO HTTP.status202
+                , expectFieldSatisfy id (not . null)


In the end, I think we can even be stronger in the assertion here. We expect exactly one transaction.

In the end, I think we can even be stronger in the assertion here. We expect exactly one transaction.

How can we be certain that there is exactly one transaction? Surely this depends on the UTxO distribution of the Byron fixture wallet? If the Byron fixture wallet has a complicated UTxO, that could easily result in more than one transaction being returned by migrateByronWallet.

If it is the case that the Byron fixture wallet has a very simple UTxO that causes only one transaction to be generated, surely that's an implementation detail that this test should not be concerned with?

Fixture wallets always have the exact same distributions (fixed by the genesis configuration) :

10 UTxOs of 10M Ada (or 1M, not sure anymore).

Also, if you use fixtureWalletWith you can chose yourself what that distribution should be (the function takes a list of coins if I recall correctly).

Fixture wallets always have the exact same distributions

There is also this wallet in genesis -> https://github.com/input-output-hk/cardano-wallet/blob/master/lib/jormungandr/test/data/jormungandr/genesis.yaml#L68-L88.

I have added this some time ago to have some variation... but I think that the utxo distribution is the same for it (i.e. 10 UTxOs), only the number of transaction is different on this particular one. (Initially it has 1 tx, whereas others have 10txs). If it is problematic or confusing then maybe it should be changed.

I think we should be cautious about making tests that depend on obscure details of mock data, unless those details are exposed as shared, top-level, reusable constants.

While it may the case that migrating the default faucet wallet just creates a single transaction, I don't see any advantage of making our test depend on that fact. I would argue that it's enough to ascertain that we have at least one transaction.

That is, unless we define the default faucet UTxO as a reusable constant, and then make the number of expected transactions a function of that constant, but I suspect that's overkill.

While it may the case that migrating the default faucet wallet just creates a single transaction, I don't see any advantage of making our test depend on that fact. I would argue that it's enough to ascertain that we have at least one transaction.

I'll introduce a fixtureByronWalletWith very much like its current dual for fixtureWallet.
With this, we'll be able to make it obvious what the UTxO size is without having to rely on a implicit knowledge on the testing setup AND, more importantly, we'll able to test a scenario where the migration actually requires more than one transaction (by creating a big UTxO, very much we do for testing the limit of the fee policy).

Yes, it is one of the faucet wallets. As far as I remember I just removed those first several funds which just creates one inital tx with 10 utxos for this single wallet (as all those addresses belong to the same wallet)... others have 10 txs with 1 utxo each (per fixture wallet)...

Ah! It'd have been better not to use a faucet wallet for this, but to define the mnemonic separately :|

I think we should be cautious about making tests that depend on details of mock data

In this particular case one aspect is that we know what to expect with our fixture wallet mock (10 txs with 10 utxos)... unless someone silently breaks this expectation (like me with introducing this variation in genesis... which I think know is not really valuable, unless we really want to test this particular case... which we currently don't).

For that reason it is maybe safer (and clearer from the test reader perspective later on) to use fixtureWalletWith where we can define utxo distribution and even the number of transactions if needed.

For that reason it is maybe safer (and clearer from the test reader perspective later on)

💯

KtorZ · 2019-11-05T08:40:29Z

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

@@ -188,7 +189,53 @@ spec = do
                    getFromResponse amount r0
            actualFee `shouldBe` predictedFee


I'd instead look for the following assertion:

-- Wait for the funds to have arrived onto the new wallet eventually $ do rNew <- request @ApiWallet ctx (getWalletEp newW) Default Empty verify rNew [ expectFieldEqual balanceAvailable (faucetAmt - fee) , expectFieldEqual balanceTotal (faucetAmt - fee) ]

Which is stronger as it shows that the estimated fee were correct and that funds have arrived on the target wallet.

@KtorZ That stronger test is available in PR #960.

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

KtorZ · 2019-11-05T23:28:22Z

Revised integration scenarios to avoid partial pattern-match on request results
Remove "fixtureByronWalletWith" and instead, use a top-level constant "fixturePassphrase"
Bumped Jörmungandr installation to current Jörmungandr's master
Adjusted Binary module for Jörmungandr to work with the recent version (more config params added, and also an extra 64 bytes in the stake delegation object which is probably some sort of authentication payload)
Cleaned-up redundant tests in Jörmungandr Binary spec already covered by the binary encode/decode roundtrip
Re-generated arbitrary genesis blocks from Jörmungandr latest master to make sure we are "rountripping" relevant formats.

cc @Anviking

KtorZ · 2019-11-05T23:28:35Z

bors r+

950: Test that `migrateByronWallet` depletes all funds from a source wallet. r=KtorZ a=jonathanknowles # Issue Number #779 # Overview This PR tests that after a successful call to `migrateByronWallet`, all funds are debited from the source wallet, leaving a balance of `0`. # Further work A further PR will test that funds are appropriately credited to the target wallet. Co-authored-by: Jonathan Knowles <[email protected]> Co-authored-by: KtorZ <[email protected]> Co-authored-by: Johannes Lund <[email protected]>

iohk-bors · 2019-11-05T23:30:05Z

Canceled

KtorZ · 2019-11-05T23:30:27Z

bors r+

950: Test that `migrateByronWallet` depletes all funds from a source wallet. r=KtorZ a=jonathanknowles # Issue Number #779 # Overview This PR tests that after a successful call to `migrateByronWallet`, all funds are debited from the source wallet, leaving a balance of `0`. # Further work A further PR will test that funds are appropriately credited to the target wallet. Co-authored-by: Jonathan Knowles <[email protected]> Co-authored-by: Johannes Lund <[email protected]> Co-authored-by: KtorZ <[email protected]>

iohk-bors · 2019-11-06T00:03:23Z

Build failed

buildkite/cardano-wallet

https://github.com/input-output-hk/cardano-wallet/wiki/Coding-Standards#limit-line-length-to-80-characters

This is consistent with the name of the operation.

…tion. The actual termination condition is that the target value is reached or exceeded. It's not required that the target value is exceeded (contrary to what the original log message implied).

This prefix better reflects the purpose of these tests.

This allows callers to specify a particular name and passphrase.

…ion.

…tion.

Also took the opportunity to clean up a bit binary specs for Jörmungandr and remove superfluous tests (already covered by the arbitrarily generate blocks)

… results

rvl · 2019-11-06T02:01:07Z

Last bors failure was due to #931 (ExitFailure 66).

jonathanknowles · 2019-11-06T02:41:14Z

bors r+

950: Test that `migrateByronWallet` migrates funds correctly. r=jonathanknowles a=jonathanknowles # Issue Number #779 # Overview This PR tests that after a successful call to `migrateByronWallet`: - [x] all funds are debited from the source wallet, leaving a balance of `0`. - [x] all funds are eventually credited to the target wallet, minus the fee. - [x] the resultant fee is the same as the fee predicted by `getByronWalletMigrateInfo`. Co-authored-by: Jonathan Knowles <[email protected]> Co-authored-by: Johannes Lund <[email protected]> Co-authored-by: KtorZ <[email protected]>

iohk-bors · 2019-11-06T03:02:19Z

Build succeeded

buildkite/cardano-wallet

jonathanknowles force-pushed the jonathanknowles/byron-migrate-tests branch from 0ad8987 to 40cafc2 Compare November 4, 2019 06:36

jonathanknowles self-assigned this Nov 4, 2019

jonathanknowles force-pushed the jonathanknowles/byron-migrate-tests branch 5 times, most recently from d40fafe to c89328f Compare November 5, 2019 08:37

jonathanknowles changed the title ~~WIP: Add tests to demonstrate that migrateByronWallet actually migrates funds.~~ Test that migrateByronWallet depletes all funds from a source wallet. Nov 5, 2019

KtorZ reviewed Nov 5, 2019

View reviewed changes

jonathanknowles mentioned this pull request Nov 5, 2019

Test that migrateByronWallet appropriately deposits funds into a target wallet. #960

Merged

2 tasks

KtorZ reviewed Nov 5, 2019

View reviewed changes

lib/core-integration/src/Test/Integration/Framework/DSL.hs Outdated Show resolved Hide resolved

KtorZ force-pushed the jonathanknowles/byron-migrate-tests branch from 43cf03f to e67d034 Compare November 5, 2019 23:24

KtorZ approved these changes Nov 5, 2019

View reviewed changes

KtorZ force-pushed the jonathanknowles/byron-migrate-tests branch from e67d034 to b681a25 Compare November 5, 2019 23:30

jonathanknowles added 8 commits November 6, 2019 01:31

Reformat code according to team coding standard.

392f040

https://github.com/input-output-hk/cardano-wallet/wiki/Coding-Standards#limit-line-length-to-80-characters

Small fixes to spelling, punctuation and grammar within comments.

6adfe65

Rename listByronWalletEp to listByronWalletsEp.

b7e80f8

This is consistent with the name of the operation.

Make log text for expectEventually reflect actual termination condi…

d1dd707

…tion. The actual termination condition is that the target value is reached or exceeded. It's not required that the target value is exceeded (contrary to what the original log message implied).

Rename BYRON_ESTIMATE test prefix to BYRON_MIGRATE.

150ecef

This prefix better reflects the purpose of these tests.

Extract out function fixtureByronWalletWith from fixtureByronWallet.

d592c69

This allows callers to specify a particular name and passphrase.

Pass through passphrase instead of duplicating a hard-coded constant.

93cfb37

Verify that all funds are depleted by the migrateByronWallet operat…

4f0c356

…ion.

jonathanknowles force-pushed the jonathanknowles/byron-migrate-tests branch from b681a25 to 5c07f79 Compare November 6, 2019 01:32

jonathanknowles changed the title ~~Test that migrateByronWallet depletes all funds from a source wallet.~~ Test that migrateByronWallet migrates funds correctly. Nov 6, 2019

jonathanknowles and others added 7 commits November 6, 2019 01:48

Verify that funds become available in the target wallet after a migra…

c11170f

…tion.

Remove sanity check for non-zero balance of Byron fixture wallet.

861bb02

Remove sanity check for non-zero balance of Byron fixture wallet.

238a985

Use the eventually combinator instead of expectEventually.

3a8645d

Bump Jörmungandr installation to eb0b7bb1

d0f718a

Also took the opportunity to clean up a bit binary specs for Jörmungandr and remove superfluous tests (already covered by the arbitrarily generate blocks)

undo fixtureByronWalletWith & remove partial pattern-match on request…

05dbbde

… results

Add full stop at the end of a comment.

c2d29c6

jonathanknowles force-pushed the jonathanknowles/byron-migrate-tests branch from bd1b894 to c2d29c6 Compare November 6, 2019 01:57

Tidy names of request results in integration tests.

9252ed0

iohk-bors bot merged commit 9252ed0 into master Nov 6, 2019

jonathanknowles deleted the jonathanknowles/byron-migrate-tests branch November 6, 2019 03:42

This was referenced Nov 6, 2019

Add API endpoint to migrate funds from a legacy wallet. #779

Closed

Add API endpoint to calculate the cost of migrating from a legacy wallet. #778

Closed

KtorZ added this to the Byron Wallet Support milestone Nov 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test that `migrateByronWallet` migrates funds correctly. #950

Test that `migrateByronWallet` migrates funds correctly. #950

jonathanknowles commented Nov 4, 2019 •

edited

Loading

KtorZ Nov 5, 2019

jonathanknowles Nov 5, 2019 •

edited

Loading

KtorZ Nov 5, 2019

jonathanknowles Nov 5, 2019

KtorZ Nov 5, 2019

jonathanknowles Nov 5, 2019

KtorZ Nov 5, 2019

piotr-iohk Nov 5, 2019 •

edited

Loading

jonathanknowles Nov 5, 2019 •

edited

Loading

KtorZ Nov 5, 2019

piotr-iohk Nov 5, 2019 •

edited

Loading

KtorZ Nov 5, 2019

piotr-iohk Nov 5, 2019

KtorZ Nov 5, 2019

KtorZ Nov 5, 2019

jonathanknowles Nov 5, 2019

KtorZ commented Nov 5, 2019

KtorZ commented Nov 5, 2019

iohk-bors bot commented Nov 5, 2019

KtorZ commented Nov 5, 2019

iohk-bors bot commented Nov 6, 2019

rvl commented Nov 6, 2019

jonathanknowles commented Nov 6, 2019

iohk-bors bot commented Nov 6, 2019

		@@ -188,7 +189,53 @@ spec = do
		getFromResponse amount r0
		actualFee `shouldBe` predictedFee

Test that migrateByronWallet migrates funds correctly. #950

Test that migrateByronWallet migrates funds correctly. #950

Conversation

jonathanknowles commented Nov 4, 2019 • edited Loading

Issue Number

Overview

Choose a reason for hiding this comment

jonathanknowles Nov 5, 2019 • 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

piotr-iohk Nov 5, 2019 • edited Loading

Choose a reason for hiding this comment

jonathanknowles Nov 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piotr-iohk Nov 5, 2019 • 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

KtorZ commented Nov 5, 2019

KtorZ commented Nov 5, 2019

iohk-bors bot commented Nov 5, 2019

Canceled

KtorZ commented Nov 5, 2019

iohk-bors bot commented Nov 6, 2019

Build failed

rvl commented Nov 6, 2019

jonathanknowles commented Nov 6, 2019

iohk-bors bot commented Nov 6, 2019

Build succeeded

Test that `migrateByronWallet` migrates funds correctly. #950

Test that `migrateByronWallet` migrates funds correctly. #950

jonathanknowles commented Nov 4, 2019 •

edited

Loading

jonathanknowles Nov 5, 2019 •

edited

Loading

piotr-iohk Nov 5, 2019 •

edited

Loading

jonathanknowles Nov 5, 2019 •

edited

Loading

piotr-iohk Nov 5, 2019 •

edited

Loading