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

Add integration tests for migration of reward balances #2655

Merged

Conversation

jonathanknowles
Copy link
Member

@jonathanknowles jonathanknowles commented May 18, 2021

Issue Number

ADP-840

Overview

This PR:

  • Adds reward-wallet-specific integration test coverage for createMigrationPlan.
    (CREATE_MIGRATION_PLAN_06)
  • Adds reward-wallet-specific integration test coverage for migrateWallet.
    (MIGRATE_09)
  • Fixes createMigrationPlan to require callers to specify a reward withdrawal method, which it passes to mkRewardAccountBuilder.
    (Previously it always passed Nothing.)
  • Fixes migrateWallet to require callers to specify a reward withdrawal method, which it passes to mkRewardAccountBuilder.
    (Previously it always passed Nothing.)
  • Fixes mkApiWalletMigrationPlan to include the reward withdrawal balance in the balanceSelected field.
    (Previously the reward balance was omitted from the balanceSelected field.

@jonathanknowles jonathanknowles force-pushed the jonathanknowles/migration-integration-tests-reward-balance branch from f9be46c to 69d95c9 Compare May 18, 2021 09:38
@jonathanknowles jonathanknowles self-assigned this May 18, 2021
This integration test checks that it's possible to create a migration
plan for a wallet that has rewards.
…withdrawal.

For Shelley-era wallets, we specify `Just SelfWithdrawal`.
For Byron-era wallets, we specify `Nothing`.

In the case of Shelley-era wallets, this causes the full reward balance
to always be included in the plan.
The node verifies that transactions are exactly balanced. Therefore, we
can rely on the contents of the `fee` field of a successfully-submitted
`ApiTransaction` to be correct.

The previous implementation was incorrect, as it failed to include the
reward withdrawal in the fee calculation.
In this test, we check that it's possible to migrate a wallet that has
rewards.
…awal.

For Shelley-era wallets, we specify `Just SelfWithdrawal`.
For Byron-era wallets, we specify `Nothing`.

In the case of Shelley-era wallets, this causes the full reward balance
to always be included in the plan, and the resultant transactions.
This echoes usage elsewhere, and clearly indicates that the selection
results are expected to have no change.
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/migration-integration-tests-reward-balance branch from 69d95c9 to d695cb0 Compare May 18, 2021 10:44
@jonathanknowles jonathanknowles marked this pull request as ready for review May 18, 2021 10:49
Copy link
Collaborator

@Anviking Anviking left a comment

Choose a reason for hiding this comment

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

lgtm!

. view #getQuantity
. view #amount
. head
. view #withdrawals
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

]

-- Check that the source wallet has been depleted:
eventually "Source wallet balance is depleted." $ do
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

extraCoinSource =
if (view #rewardWithdrawal migrationSelection) > Coin 0
then Just (view #rewardWithdrawal migrationSelection)
else Nothing
Copy link
Member Author

Choose a reason for hiding this comment

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

We should add a comment here explaining why this is necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in c3a8bfd.

@jonathanknowles
Copy link
Member Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 18, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit 1d100e5 into master May 18, 2021
@iohk-bors iohk-bors bot deleted the jonathanknowles/migration-integration-tests-reward-balance branch May 18, 2021 14:24
Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

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

Nice one 👍

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.

None yet

3 participants