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

Migration endpoints for shelley and cardano-node's byron support #1678

Merged
merged 21 commits into from
May 26, 2020
Merged

Migration endpoints for shelley and cardano-node's byron support #1678

merged 21 commits into from
May 26, 2020

Conversation

paweljakubas
Copy link
Contributor

@paweljakubas paweljakubas commented May 21, 2020 •
edited
Loading

Issue Number

#1675

Overview

  • I have make ApiWalletMigrationPostData more flexible
  • I have added ShelleyMigration to API enpoints
  • I have added unit tests for the changed ApiWalletMigrationPostData
  • I have added integration tests to cardano-node's byron suite

Comments

@paweljakubas paweljakubas self-assigned this May 21, 2020
@paweljakubas paweljakubas changed the title Migration endpoints for shelley Migration endpoints for shelley and cardano-node's byron support May 21, 2020
jonathanknowles
jonathanknowles requested changes May 22, 2020
View reviewed changes
lib/byron/src/Cardano/Wallet/Byron/Api/Server.hs Outdated Show resolved Hide resolved
lib/byron/src/Cardano/Wallet/Byron/Api/Server.hs Show resolved Hide resolved
lib/core-integration/src/Test/Integration/Scenario/API/Byron/Wallets.hs Outdated Show resolved Hide resolved
lib/core-integration/src/Test/Integration/Scenario/API/Byron/Wallets.hs Outdated Show resolved Hide resolved
lib/core-integration/src/Test/Integration/Scenario/API/Byron/Wallets.hs Outdated Show resolved Hide resolved
lib/core/test/unit/Cardano/Wallet/Api/TypesSpec.hs Show resolved Hide resolved
lib/core/test/unit/Cardano/Wallet/Api/TypesSpec.hs Show resolved Hide resolved
lib/core/test/unit/Cardano/Wallet/Api/TypesSpec.hs Show resolved Hide resolved
specifications/api/swagger.yaml Show resolved Hide resolved
specifications/api/swagger.yaml Outdated Show resolved Hide resolved
@rvl rvl self-assigned this May 22, 2020
@paweljakubas paweljakubas force-pushed the paweljakubas/1675/migration-endpoints-for-shelley branch from 2fed8d0 to 5bedb75 Compare May 25, 2020 14:33
@paweljakubas paweljakubas marked this pull request as ready for review May 25, 2020 14:35
@paweljakubas
Copy link
Contributor Author

bors try

iohk-bors bot added a commit that referenced this pull request May 25, 2020
@iohk-bors
Try #1678:
87744db
@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 25, 2020

try

Build failed

@paweljakubas
Copy link
Contributor Author

bors try

iohk-bors bot added a commit that referenced this pull request May 25, 2020
@iohk-bors
Try #1678:
a771904
@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 25, 2020

try

Build failed

@paweljakubas
Copy link
Contributor Author

bors try

iohk-bors bot added a commit that referenced this pull request May 25, 2020
@iohk-bors
Try #1678:
ab04db0
@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 25, 2020

try

Build failed

@paweljakubas paweljakubas force-pushed the paweljakubas/1675/migration-endpoints-for-shelley branch from 460a26b to 1ced010 Compare May 25, 2020 16:52
@paweljakubas
Copy link
Contributor Author

bors try

iohk-bors bot added a commit that referenced this pull request May 25, 2020
@iohk-bors
Try #1678:
21a02aa
@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 25, 2020

try

Build failed

rvl and others added 10 commits May 25, 2020 19:24
@rvl @paweljakubas
swagger: Fix getShelleyWalletMigrationInfo operationId
f98296e
@rvl @jonathanknowles @paweljakubas
Update lib/core-integration/src/Test/Integration/Scenario/API/Byron/W…
9b92742 
…allets.hs

Co-authored-by: Jonathan Knowles <[email protected]>
@rvl @jonathanknowles @paweljakubas
Update lib/byron/src/Cardano/Wallet/Byron/Api/Server.hs
95ee3ae 
Co-authored-by: Jonathan Knowles <[email protected]>
@rvl @paweljakubas
Reduce TypesSpec generator duplication
14bc7e2
@rvl @jonathanknowles @paweljakubas
Update lib/core-integration/src/Test/Integration/Scenario/API/Byron/W…
953067b 
…allets.hs

Co-authored-by: Jonathan Knowles <[email protected]>
@rvl @jonathanknowles @paweljakubas
Update lib/core-integration/src/Test/Integration/Scenario/API/Byron/W…
8f29d9e 
…allets.hs

Co-authored-by: Jonathan Knowles <[email protected]>
@rvl @paweljakubas
Fix tests compile
ef3fcfa
@paweljakubas
address very subtle max address size bug - it needs to bey byron key
c2f6314
@paweljakubas
realign all arguments in Server.hs in byron, shelley and jormungandr
577f98b
@paweljakubas
correct previous wrong merge conflict
2eaa3d3
@paweljakubas paweljakubas force-pushed the paweljakubas/1675/migration-endpoints-for-shelley branch from 1ced010 to cf11d5a Compare May 25, 2020 17:25
jonathanknowles
jonathanknowles approved these changes May 26, 2020
View reviewed changes
Copy link
Member

@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 fine to me.

@rvl
Copy link
Contributor

rvl commented May 26, 2020

The Fee estimation calculation unit tests for Icarus / Mainnet and Icarus / Testnet fail after fixing the MaxSizeOf Address n ByronKey constraint on newTransactionLayer.

@KtorZ
Copy link
Member

KtorZ commented May 26, 2020

Which makes sense since the tests and assertions were tied to the type of key. Our estimation will actually be way worse than it was due to this change. So, we need to increase the margin here:

@paweljakubas
Copy link
Contributor Author

paweljakubas commented May 26, 2020 •
edited
Loading

@rvl @KtorZ I made a fix -> cf11d5a
and checked 10x stack test cardano-wallet-byron:unit locally and 10x GREEN

@paweljakubas
Copy link
Contributor Author

bors try

iohk-bors bot added a commit that referenced this pull request May 26, 2020
@iohk-bors
Try #1678:
ee4001b
@KtorZ
Copy link
Member

KtorZ commented May 26, 2020

bors r-

@KtorZ
Copy link
Member

KtorZ commented May 26, 2020 

  1) Cardano.Wallet.Byron.Transaction, Fee estimation calculation, Icarus / Testnet
       Falsifiable (after 122 tests and 7 shrinks):
         real size inf:   1247
         real size sup:   1412
         estimated size:  1414

The new margin is not sufficient because it does not include the old margin. It should be 12 + 33 = 45 bytes and not only 33! Because, we now make an error on the address type, but we still make the approximation error on other components of the addresses (the internal crc32 for instance).

@paweljakubas
correct byron unit test
68932bb 
adjust restore byron

increase upper bound for addresses in TransactionSpec
@paweljakubas paweljakubas force-pushed the paweljakubas/1675/migration-endpoints-for-shelley branch from cf11d5a to 68932bb Compare May 26, 2020 08:17
@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 26, 2020

try

Build failed

@paweljakubas
Copy link
Contributor Author

bors try

iohk-bors bot added a commit that referenced this pull request May 26, 2020
@iohk-bors
Try #1678:
b321b1c
@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 26, 2020

try

Build failed

@paweljakubas
Copy link
Contributor Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 26, 2020

Build succeeded

@iohk-bors iohk-bors bot merged commit 0318359 into master May 26, 2020
@iohk-bors iohk-bors bot deleted the paweljakubas/1675/migration-endpoints-for-shelley branch May 26, 2020 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonathanknowles jonathanknowles jonathanknowles approved these changes

@KtorZ KtorZ Awaiting requested review from KtorZ

@rvl rvl Awaiting requested review from rvl

Assignees

@rvl rvl

@paweljakubas paweljakubas

Labels
None yet
Projects
None yet
Milestone
(ADP-257) API endpoint: 'sendAll' fun...
Development

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