API specification for multi-asset wallet migration #2590

jonathanknowles · 2021-03-31T09:03:43Z

Issue Number

ADP-840

Overview

This PR revises the API specification to support multi-asset wallet migrations.

The server implementation will be added in a subsequent PR. (Connecting the API with the algorithm.)

Details

This PR:

Adds a ApiWalletMigrationBalance type:

ApiWalletMigrationBalance: &ApiWalletMigrationBalance
  type: object
  required:
    - ada
    - assets
  properties:
    ada: *amount
    assets: *walletAssets

Revises the ApiWalletMigrationInfo type:

ApiWalletMigrationInfo: &ApiWalletMigrationInfo
  type: object
  required:
    - selections
    - total_fee
    - balance_leftover
    - balance_selected
  properties:
    selections:
      type: array
      items: *ApiCoinSelection
    total_fee:
      <<: *amount
    balance_leftover:
      <<: *ApiWalletMigrationBalance
    balance_selected:
      <<: *ApiWalletMigrationBalance

lib/core/src/Cardano/Wallet/Api/Types.hs

Anviking

Looks reasonable, I think

lib/core/src/Cardano/Wallet/Api/Types.hs

specifications/api/swagger.yaml

In response to review feedback: #2590 (comment)

lib/core/test/unit/Cardano/Wallet/Api/TypesSpec.hs

rvl

There should also be updates to the getShelleyWalletMigrationInfo and migrateShelleyWallet specs, right?

specifications/api/swagger.yaml

jonathanknowles · 2021-04-29T13:06:13Z

Hi @rvl

There should also be updates to the getShelleyWalletMigrationInfo

I plan to update the description of this endpoint in the next PR, when the algorithm is connected up to the API. (We'll have to update the description then anyway, in order to remove the "⚠️ IMPORTANT: this endpoint has been temporarily disabled!" warning.)

As for their return types, these currently both have the same return types:

getShelleyWalletMigrationInfo
getByronWalletMigrationInfo

These both return a responsesGetWalletMigrationInfo, which returns a ApiWalletMigrationInfo. I think it's okay for both to return the same type. In the Byron case, the assets part of each balance field will be empty.

and migrateShelleyWallet specs, right?

I wasn't planning to change the type of this endpoint in any way, as it currently returns a list of transactions, which seems to be exactly what we need.

But perhaps you've thought of something that I've missed?

rvl · 2021-04-29T13:18:50Z

In migrateShelleyWallet the description mentions "shape", but this no longer applies. It could also be revised to say that it transfers as much funds as possible.

The description of getShelleyWalletMigrationInfo could also be revised. Its summary is "Calculate Cost", which is not quite right any more.

jonathanknowles · 2021-04-29T13:55:08Z

In migrateShelleyWallet the description mentions "shape", but this no longer applies. It could also be revised to say that it transfers as much funds as possible.

The description of getShelleyWalletMigrationInfo could also be revised. Its summary is "Calculate Cost", which is not quite right any more.

Hi @rvl

I've updated the descriptions in this pair of commits:

7cdff84: migrate{Byron,Shelley}Wallet
76c0867: get{Byron,Shelley}WalletMigrationInfo

We'll eventually need the `ApiWalletMigrationInfo` record to encode two different types of balance: - balanceSelected - balanceLeftover

rvl

Nice thanks - better now.

rvl · 2021-04-30T02:53:45Z

bors r+

2590: API specification for multi-asset wallet migration r=rvl a=jonathanknowles # Issue Number ADP-840 # Overview This PR revises the **_API specification_** to support multi-asset wallet migrations. The **_server implementation_** will be added in a subsequent PR. (Connecting the API with the algorithm.) # Details This PR: - [x] Adds a `ApiWalletMigrationBalance` type: ```yaml ApiWalletMigrationBalance: &ApiWalletMigrationBalance type: object required: - ada - assets properties: ada: *amount assets: *walletAssets ``` - [x] Revises the `ApiWalletMigrationInfo` type: ```yaml ApiWalletMigrationInfo: &ApiWalletMigrationInfo type: object required: - selections - total_fee - balance_leftover - balance_selected properties: selections: type: array items: *ApiCoinSelection total_fee: <<: *amount balance_leftover: <<: *ApiWalletMigrationBalance balance_selected: <<: *ApiWalletMigrationBalance ``` Co-authored-by: Jonathan Knowles <[email protected]>

iohk-bors · 2021-04-30T04:20:18Z

Build failed:

buildkite/cardano-wallet

Looks like something weird happened while attempting to start the build.

(i.e., not an actual test failure.)

jonathanknowles · 2021-04-30T04:37:14Z

bors r+

rvl · 2021-04-30T04:41:44Z

Right, that bors failure was correct. This was the error message:

$ ././.buildkite/check-bors.sh
master commit is 182e86427dad393fc4f97037d4a9faebc5d95743
bors/staging parent commits are e687560265a0a49d0f3d5056969ec5b0c0288afa 3e14fb4abc2da149242656f51006d12a51f4b479
 
Refusing to merge because the pull request does not target master.
You should only use Bors to merge into the master branch.
Either change the PR base branch to master, or merge manually.

jonathanknowles · 2021-04-30T04:51:08Z

Right, that bors failure was correct. This was the error message:

Perhaps it's because of a race condition?

The branch of this PR was originally based on jonathanknowles/multi-asset-migration-transaction-layer, which just merged to master (#2620).

If branch A is based on branch B which is based on master, but then branch B is merged to master, then I've observed that GitHub will automatically update the base branch of A to be master.

But if bors attempts to build before this happens, then it will fetch something that's not yet based on master, which seems to be what happened here.

When I did bors r+ for the second time, I didn't have to do any manual rebasing onto master -- it seems that GitHub did this automatically.

rvl · 2021-04-30T05:20:31Z

Yes - a race, sort of. Bors would have created the merge commit for the next batch before it got notification from GitHub that the PR base branch changed.

iohk-bors · 2021-04-30T05:43:49Z

Build succeeded:

jonathanknowles force-pushed the jonathanknowles/multi-asset-migration-api branch from 246150e to 16f7eb4 Compare March 31, 2021 09:40

jonathanknowles self-assigned this Mar 31, 2021

jonathanknowles requested review from Anviking, rvl, piotr-iohk and KtorZ March 31, 2021 10:13

piotr-iohk reviewed Mar 31, 2021

View reviewed changes

lib/core/src/Cardano/Wallet/Api/Types.hs Outdated Show resolved Hide resolved

Anviking reviewed Mar 31, 2021

View reviewed changes

lib/core/src/Cardano/Wallet/Api/Types.hs Outdated Show resolved Hide resolved

specifications/api/swagger.yaml Outdated Show resolved Hide resolved

jonathanknowles added a commit that referenced this pull request Apr 1, 2021

Rename ApiWalletMigration.cost to ApiWalletMigration.totalFee.

0369671

In response to review feedback: #2590 (comment)

jonathanknowles force-pushed the jonathanknowles/multi-asset-migration-api branch from 23594eb to f21643f Compare April 29, 2021 05:11

jonathanknowles changed the base branch from master to jonathanknowles/multi-asset-migration-transaction-layer April 29, 2021 05:12

jonathanknowles force-pushed the jonathanknowles/multi-asset-migration-api branch from f21643f to 65e154b Compare April 29, 2021 05:14

jonathanknowles changed the title ~~Revise API to support multi-asset wallet migrations~~ Revise API specification to support multi-asset wallet migrations Apr 29, 2021

jonathanknowles requested review from Anviking and piotr-iohk and removed request for rvl and KtorZ April 29, 2021 05:24

KtorZ reviewed Apr 29, 2021

View reviewed changes

lib/core/test/unit/Cardano/Wallet/Api/TypesSpec.hs Outdated Show resolved Hide resolved

jonathanknowles force-pushed the jonathanknowles/multi-asset-migration-api branch from 65e154b to 34f56cd Compare April 29, 2021 05:33

jonathanknowles changed the title ~~Revise API specification to support multi-asset wallet migrations~~ API specification for multi-asset wallet migration Apr 29, 2021

jonathanknowles marked this pull request as ready for review April 29, 2021 08:23

rvl reviewed Apr 29, 2021

View reviewed changes

specifications/api/swagger.yaml Show resolved Hide resolved

jonathanknowles force-pushed the jonathanknowles/multi-asset-migration-transaction-layer branch from 2099f94 to c9470ca Compare April 29, 2021 12:27

jonathanknowles force-pushed the jonathanknowles/multi-asset-migration-api branch 2 times, most recently from 697f0f2 to 202e024 Compare April 29, 2021 12:50

jonathanknowles force-pushed the jonathanknowles/multi-asset-migration-api branch from c650885 to 5e9a20a Compare April 29, 2021 13:52

jonathanknowles force-pushed the jonathanknowles/multi-asset-migration-api branch from 5e9a20a to 76c0867 Compare April 29, 2021 13:59

jonathanknowles force-pushed the jonathanknowles/multi-asset-migration-transaction-layer branch from c9470ca to e687560 Compare April 30, 2021 00:11

jonathanknowles added 10 commits April 30, 2021 00:12

[Reformatting] Tidy up migration part of Api.TypesSpec.

c2895de

Add ApiWalletMigrationBalance type.

6842c6e

Rename field leftovers to balanceLeftover.

39e8c96

We'll eventually need the `ApiWalletMigrationInfo` record to encode two different types of balance: - balanceSelected - balanceLeftover

Change type of balanceLeftover to ApiWalletMigrationBalance.

f7035e0

Add balanceSelected field to ApiWalletMigrationInfo.

de407c5

Rename migration_cost to total_fee.

c5fc268

Add selections field to ApiWalletMigrationInfo.

0c3e3a4

Add documentation to specification for ApiWalletMigrationInfo.

10a49aa

Revise migrate{Byron,Shelley}Wallet endpoint descriptions.

d28e5e6

Revise get{Byron,Shelley}WalletMigrationInfo endpoint descriptions.

3e14fb4

jonathanknowles force-pushed the jonathanknowles/multi-asset-migration-api branch from 76c0867 to 3e14fb4 Compare April 30, 2021 00:12

rvl approved these changes Apr 30, 2021

View reviewed changes

Base automatically changed from jonathanknowles/multi-asset-migration-transaction-layer to master April 30, 2021 04:19

iohk-bors bot merged commit bdd0d26 into master Apr 30, 2021

iohk-bors bot deleted the jonathanknowles/multi-asset-migration-api branch April 30, 2021 05:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API specification for multi-asset wallet migration #2590

API specification for multi-asset wallet migration #2590

jonathanknowles commented Mar 31, 2021 •

edited

Loading

Anviking left a comment

rvl left a comment

jonathanknowles commented Apr 29, 2021

rvl commented Apr 29, 2021

jonathanknowles commented Apr 29, 2021 •

edited

Loading

rvl left a comment

rvl commented Apr 30, 2021

iohk-bors bot commented Apr 30, 2021 •

edited by jonathanknowles

Loading

jonathanknowles commented Apr 30, 2021

rvl commented Apr 30, 2021

jonathanknowles commented Apr 30, 2021

rvl commented Apr 30, 2021

iohk-bors bot commented Apr 30, 2021

API specification for multi-asset wallet migration #2590

API specification for multi-asset wallet migration #2590

Conversation

jonathanknowles commented Mar 31, 2021 • edited Loading

Issue Number

Overview

Details

Anviking left a comment

Choose a reason for hiding this comment

rvl left a comment

Choose a reason for hiding this comment

jonathanknowles commented Apr 29, 2021

rvl commented Apr 29, 2021

jonathanknowles commented Apr 29, 2021 • edited Loading

rvl left a comment

Choose a reason for hiding this comment

rvl commented Apr 30, 2021

iohk-bors bot commented Apr 30, 2021 • edited by jonathanknowles Loading

jonathanknowles commented Apr 30, 2021

rvl commented Apr 30, 2021

jonathanknowles commented Apr 30, 2021

rvl commented Apr 30, 2021

iohk-bors bot commented Apr 30, 2021

jonathanknowles commented Mar 31, 2021 •

edited

Loading

jonathanknowles commented Apr 29, 2021 •

edited

Loading

iohk-bors bot commented Apr 30, 2021 •

edited by jonathanknowles

Loading