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.

Sign up for GitHub

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

Jump to bottom

Add sendAll for Byron endpoints #1664

Merged
merged 15 commits into from
May 20, 2020
Merged

Add sendAll for Byron endpoints #1664

merged 15 commits into from
May 20, 2020

Conversation

paweljakubas
Copy link
Contributor

@paweljakubas paweljakubas commented May 14, 2020 •
edited
Loading

Issue Number

#1658

Overview

  • I have added refactored endpoints in ByronMigration family
  • I have changed swagger
  • I have changed impl of migrateWallet
  • I have updated unit tests
  • I have updated integration tests
  • I have added properties for assignMigrationAddresses

Comments

How to run tests :

stack test cardano-wallet-jormungandr:jormungandr-integration --ta '-m "BYRON_MIGRATE_01 -  migrate a big"'

@paweljakubas paweljakubas self-assigned this May 14, 2020
@paweljakubas paweljakubas added ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG and removed FEATURE REQUEST labels May 14, 2020
@paweljakubas paweljakubas force-pushed the paweljakubas/1658/add-sendAll-endpoint branch from 2ae5a58 to 3fbbf5c Compare May 14, 2020 17:04
@paweljakubas paweljakubas changed the title Add type scaffolding and swagger support for sendAll endpoints Add sendAll for Byron endpoints May 14, 2020
@paweljakubas paweljakubas force-pushed the paweljakubas/1658/add-sendAll-endpoint branch 3 times, most recently from ba3fabb to eeeabfc Compare May 15, 2020 15:48
@KtorZ KtorZ mentioned this pull request May 15, 2020
Merged
@paweljakubas paweljakubas force-pushed the paweljakubas/1658/add-sendAll-endpoint branch from eeeabfc to 71433ba Compare May 18, 2020 17:42
jonathanknowles
jonathanknowles suggested changes May 19, 2020
View reviewed changes
Copy link
Contributor

@jonathanknowles jonathanknowles left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hi Pawel

This is looking good so far!

I've left a few comments already.

I do have one request: to write some property tests for the changeCoinSelectionForMigration function.

Some properties to consider:

  1. Selection count should be preserved:
    The number of created transactions should be the same as the number of selections.
  2. Coin values should be preserved:
    For each transaction t created from a selection s, the coin values within t should be identical to the change coin values within s. (The counts and values of coins should both be identical.)
  3. Addresses should be recycled fairly:
    For any given pair of addresses a1 and a2 in the given address list, if a1 is used n times, then a2 should be used either n or n − 1 times. (Assuming a1 and a2 appear in order.)

Let me know if I can provide any assistance with this! :)

@paweljakubas
Copy link
Contributor Author

bors try

iohk-bors bot added a commit that referenced this pull request May 19, 2020
@iohk-bors
Try #1664:
6865adb
@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 19, 2020

try

Build succeeded

rvl
rvl approved these changes May 20, 2020
View reviewed changes
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.

Looks good.

jonathanknowles
jonathanknowles suggested changes May 20, 2020
View reviewed changes
Copy link
Contributor

@jonathanknowles jonathanknowles left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hi Pawel

Thanks for addressing the previous review feedback!

Your property tests for assignMigrationAddresses look really nice. 👍

I have one suggestion:

  • In the prop_fairAddressesRecycled property, it might be helpful to add a counterexample string, to help us debug if the test fails (since the expected result is just a Boolean).

I also have one request:

  • Is it possible to adjust your changes to lib/core-integration/src/Test/Integration/Scenario/API/Shelley/Wallets.hs so that they comply with the line length limit?

    I've added code suggestions in comments: applying these should fix these issues. 👍

Thanks!

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/Wallets.hs Outdated Show resolved Hide resolved
lib/core-integration/src/Test/Integration/Scenario/API/Shelley/Wallets.hs Outdated Show resolved Hide resolved
lib/core-integration/src/Test/Integration/Scenario/API/Shelley/Wallets.hs Outdated Show resolved Hide resolved
lib/core-integration/src/Test/Integration/Scenario/API/Shelley/Wallets.hs Outdated Show resolved Hide resolved
lib/core-integration/src/Test/Integration/Scenario/API/Shelley/Wallets.hs Outdated Show resolved Hide resolved
lib/core/test/unit/Cardano/Wallet/Primitive/CoinSelectionSpec.hs Show resolved Hide resolved
lib/core/test/unit/Cardano/Wallet/Primitive/CoinSelectionSpec.hs Show resolved Hide resolved
lib/core/test/unit/Cardano/Wallet/Primitive/CoinSelectionSpec.hs Outdated Show resolved Hide resolved
lib/core/test/unit/Cardano/Wallet/Primitive/CoinSelectionSpec.hs Outdated Show resolved Hide resolved
lib/core/test/unit/Cardano/Wallet/Primitive/CoinSelectionSpec.hs Outdated Show resolved Hide resolved
@paweljakubas paweljakubas force-pushed the paweljakubas/1658/add-sendAll-endpoint branch from 9004230 to cb4f1a7 Compare May 20, 2020 10:51
iohk-bors bot added a commit that referenced this pull request May 20, 2020
@iohk-bors @KtorZ
Merge #1669
f397080 
1669: Prepare room for Shelley addresses r=KtorZ a=KtorZ

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->

#1672

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->
- f98b904
  📍 **cleanup redundant constraints from the wallet engine**
  
- 9524ccc
  📍 **remove some hard-coded 'ShelleyKey' in favor of more flexible constraints**
  
- c8bada8
  📍 **rename AddressDerivation.Shelley & ShelleyKey to Jormungandr**
  Better reflect what this module really is and what it really targets. Makes room for the iminent shelley address support.

- 5bd26c6
  📍 **Regenerate nix**
  
- 2c27166
  📍 **add partial implementation for Shelley keys, for type-checking**
  
- a767ac2
  📍 **replace occurences of 'ShelleyKey' with 'JormungandrKey' in relevant integration tests**
  
- 9d5c3fd
  📍 **implement bits about fingerprints for ShelleyKey**

# Comments

<!-- Additional comments or screenshots to attach if any -->

Based on #1664 

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: KtorZ <[email protected]>
Co-authored-by: IOHK <[email protected]>
jonathanknowles
jonathanknowles approved these changes May 20, 2020
View reviewed changes
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.

Looks good to me!

@paweljakubas paweljakubas force-pushed the paweljakubas/1658/add-sendAll-endpoint branch from cb4f1a7 to 337778a Compare May 20, 2020 14:54
@paweljakubas
Copy link
Contributor Author

bors r+

iohk-bors bot added a commit that referenced this pull request May 20, 2020
@iohk-bors @paweljakubas
Merge #1664
c93e6f9 
1664: Add sendAll for Byron endpoints r=paweljakubas a=paweljakubas

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->
#1658 

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] I have added refactored endpoints in ByronMigration family
- [x] I have changed swagger 
- [x] I have changed impl of migrateWallet
- [x] I have updated unit tests 
- [x] I have updated integration tests
- [x] I have added properties for `assignMigrationAddresses`

# Comments

<!-- Additional comments or screenshots to attach if any -->

How to run tests : 
```
stack test cardano-wallet-jormungandr:jormungandr-integration --ta '-m "BYRON_MIGRATE_01 -  migrate a big"'
```

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Pawel Jakubas <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 20, 2020

Build failed

paweljakubas added 15 commits May 20, 2020 18:32
@paweljakubas @KtorZ
add MigrateWallet to Wallets
ad80b12 
add more

add corresponding entry to swagger.yaml

add migration cost to swagger and prepare for reuse of ApiByronWalletMigrationInfo - with name changed

add corresponding byron wallets endpoints

update swagger to new byron migrations

deal with byron migration only
@paweljakubas @KtorZ
reimplement migrateWallet
72f731f
@paweljakubas @KtorZ
adjust cardano-wallet-core:unit tests
89a2ffe
@paweljakubas @KtorZ
refactor ApiWalletMigrateData and add malformed cases plus Arbitrary …
c2b565c 
…instances

correct swagger
@paweljakubas @KtorZ
make stack build compiling
3caf1be
@paweljakubas @KtorZ
adjust rest Wallet core-integration tests
b134294
@paweljakubas @KtorZ
fix bug making big wallet migration fail
55619b2
@paweljakubas @KtorZ
comply with PR review suggestions
8e00a20
@paweljakubas @KtorZ
selections preserved property for assignMigrationAddresses
1a0398f
@paweljakubas @KtorZ
coin values preserved property for assignMigrationAddresses
df4a0b5
@paweljakubas @KtorZ
all inputs are used property for assignMigrationAddresses
bc541d2
@paweljakubas @KtorZ
add per tx properties for assignMigrationAddresses
b6025c0
@paweljakubas @KtorZ
addresses recycled properties for assignMigrationAddresses
4e2b23d
@paweljakubas @KtorZ
final polishing
da8a690
@paweljakubas @KtorZ
last bit left after merge conflict resolution
767dc63
@KtorZ KtorZ force-pushed the paweljakubas/1658/add-sendAll-endpoint branch from 337778a to 767dc63 Compare May 20, 2020 16:33
@KtorZ
Copy link
Member

KtorZ commented May 20, 2020

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 20, 2020

Build succeeded

@iohk-bors iohk-bors bot merged commit 6aaa052 into master May 20, 2020
@iohk-bors iohk-bors bot deleted the paweljakubas/1658/add-sendAll-endpoint branch May 20, 2020 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rvl rvl rvl approved these changes

@jonathanknowles jonathanknowles jonathanknowles approved these changes

Assignees

@paweljakubas paweljakubas

Labels
ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG
Projects
None yet
Milestone
(ADP-257) API endpoint: 'sendAll' fun...
Development

Successfully merging this pull request may close these issues.
4 participants
@paweljakubas @KtorZ @jonathanknowles @rvl