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

Import legacy Byron addresses #1645

Merged
merged 8 commits into from
May 11, 2020
Merged

Import legacy Byron addresses #1645

merged 8 commits into from
May 11, 2020

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented May 8, 2020

Issue Number

ADP-292

Overview

  • f71d8a8
    📍 add 'putRandomAddress' to the swagger specifications and API servant definition

  • db29bab
    📍 write dummy server implementation for 'putRandomAddress' and fill the gap in various APIs

  • 46aa7c5
    📍 add integration tests for byron address restoration

  • 3f255e7
    📍 implement server and application handlers for importing an address

  • f7ba8fb
    📍 add command for 'address import' mapping to the newly introduced endpoint

  • 2ae81f6
    📍 add some integration tests scenario for 'address import'

  • 73294ae
    📍 fix non-atomic checkpoint modification in {import,create}RandomAddress
    This is subtle but, everytime we are tempted to write:

    cp <- atomically $ readCheckpoint wid
    {- Do some stuff... -}
    atomically $ putCheckpoint wid cp'

    we should in reality really go for:

    atomically $ do
      cp <- readCheckpoint wid
      {- Do some stuff... -}
      putCheckpoint wid cp'

    because there's no guarantee that the checkpoint hasn't already
    changed between the moment we fetched it, and the moment we insert a
    modification. This could have disastreous effects like actually
    updating a checkpoint in the past while a new one has already been
    created with now oudated data; or worse, in the case of EBB, it'll
    simply override the newly written checkpoint and makes the database
    silently roll back of exactly 1 block!

Comments

Screenshot from 2020-05-08 18-05-25

$ cardano-wallet-byron address import --help
Usage: cardano-wallet-byron address import [--port INT] WALLET_ID ADDRESS
  Import a random address generated elsewhere. Only available for random
  wallets. The address must belong to the target wallet.

Available options:
  -h,--help                Show this help text
  --port INT               port used for serving the wallet API. (default: 8090)

@KtorZ KtorZ added the ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG label May 8, 2020
@KtorZ KtorZ requested a review from paweljakubas May 8, 2020 16:05
@KtorZ KtorZ self-assigned this May 8, 2020
@KtorZ
Copy link
Member Author

KtorZ commented May 8, 2020

bors try

iohk-bors bot added a commit that referenced this pull request May 8, 2020
@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 8, 2020

try

Build failed

@KtorZ
Copy link
Member Author

KtorZ commented May 8, 2020

bors r+

iohk-bors bot added a commit that referenced this pull request May 8, 2020
1645: Import legacy Byron addresses r=KtorZ a=KtorZ

# Issue Number

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

ADP-292

# Overview

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

- f71d8a8
  📍 **add 'putRandomAddress' to the swagger specifications and API servant definition**
  
- db29bab
  📍 **write dummy server implementation for 'putRandomAddress' and fill the gap in various APIs**
  
- 46aa7c5
  📍 **add integration tests for byron address restoration**
  
- 3f255e7
  📍 **implement server and application handlers for importing an address**
  
- f7ba8fb
  📍 **add command for 'address import' mapping to the newly introduced endpoint**
  
- 2ae81f6
  📍 **add some integration tests scenario for 'address import'**

# Comments

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

![Screenshot from 2020-05-08 18-05-25](https://user-images.githubusercontent.com/5680256/81424623-89014c00-9156-11ea-9a33-a4e6fb3fcd50.png)

```
$ cardano-wallet-byron address import --help
Usage: cardano-wallet-byron address import [--port INT] WALLET_ID ADDRESS
  Import a random address generated elsewhere. Only available for random
  wallets. The address must belong to the target wallet.

Available options:
  -h,--help                Show this help text
  --port INT               port used for serving the wallet API. (default: 8090)
```

<!-- 
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]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 8, 2020

Build failed

@KtorZ
Copy link
Member Author

KtorZ commented May 8, 2020

bors r+

iohk-bors bot added a commit that referenced this pull request May 8, 2020
1645: Import legacy Byron addresses r=KtorZ a=KtorZ

# Issue Number

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

ADP-292

# Overview

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

- f71d8a8
  📍 **add 'putRandomAddress' to the swagger specifications and API servant definition**
  
- db29bab
  📍 **write dummy server implementation for 'putRandomAddress' and fill the gap in various APIs**
  
- 46aa7c5
  📍 **add integration tests for byron address restoration**
  
- 3f255e7
  📍 **implement server and application handlers for importing an address**
  
- f7ba8fb
  📍 **add command for 'address import' mapping to the newly introduced endpoint**
  
- 2ae81f6
  📍 **add some integration tests scenario for 'address import'**

- 73294ae
  📍 **fix non-atomic checkpoint modification in {import,create}RandomAddress**
    This is subtle but, everytime we are tempted to write:

  ```hs
  cp <- atomically $ readCheckpoint wid
  {- Do some stuff... -}
  atomically $ putCheckpoint wid cp'
  ```

  we should in reality really go for:

  ```hs
  atomically $ do
    cp <- readCheckpoint wid
    {- Do some stuff... -}
    putCheckpoint wid cp'
  ```

  because there's no guarantee that the checkpoint hasn't already
  changed between the moment we fetched it, and the moment we insert a
  modification. This could have disastreous effects like actually
  updating a checkpoint in the past while a new one has already been
  created with now oudated data; or worse, in the case of EBB, it'll
  simply override the newly written checkpoint and makes the database
  silently roll back of exactly 1 block!

# Comments

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

![Screenshot from 2020-05-08 18-05-25](https://user-images.githubusercontent.com/5680256/81424623-89014c00-9156-11ea-9a33-a4e6fb3fcd50.png)

```
$ cardano-wallet-byron address import --help
Usage: cardano-wallet-byron address import [--port INT] WALLET_ID ADDRESS
  Import a random address generated elsewhere. Only available for random
  wallets. The address must belong to the target wallet.

Available options:
  -h,--help                Show this help text
  --port INT               port used for serving the wallet API. (default: 8090)
```

<!-- 
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]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 8, 2020

Build failed

  This is subtle but, everytime we are tempted to write:

  ```hs
  cp <- atomically $ readCheckpoint wid
  {- Do some stuff... -}
  atomically $ putCheckpoint wid cp'
  ```

  we should in reality really go for:

  ```hs
  atomically $ do
    cp <- readCheckpoint wid
    {- Do some stuff... -}
    putCheckpoint wid cp'
  ```

  because there's no guarantee that the checkpoint hasn't already
  changed between the moment we fetched it, and the moment we insert a
  modification. This could have disastreous effects like actually
  updating a checkpoint in the past while a new one has already been
  created with now oudated data; or worse, in the case of EBB, it'll
  simply override the newly written checkpoint and makes the database
  silently roll back of exactly 1 block!
@KtorZ KtorZ force-pushed the KtorZ/ADP-292/import-addresses branch from 73294ae to b7ac9ad Compare May 10, 2020 21:33
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.

@@ -2031,6 +2047,22 @@ paths:
schema: *ApiPostRandomAddressData
responses: *responsesPostRandomAddress

/byron-wallets/{walletId}/addresses/{addressId}:
put:
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't the REST dogmas say that PUT is for updating and POST is for creating?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not necessarily. PUT is for pushing an already created resource assuming that a client does have the ability to create such resource entirely. This is the case here where the address pre-exists to the call, and is therefore pushed to the server.

POST would be for creating a resource from partial data, or from a "constructor". There are resources where this is not possible to do differently, like the wallet. We can't possibly allow a client to "put" its own balance or sync progress, hence the POST.

Copy link
Member Author

Choose a reason for hiding this comment

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

Then, for choosing between PUT and PATCH, it's more of a matter of perspective. We are not using PATCH much in the API where it would be legitimate IMO (like when updating the passphrase of metadata, i.e. when doing a partial update of a resource). Yet, we've stick to PUT here because people tend to be somewhat confused by the PATCH method. And, from the perspective of a sub-resource, a PUT is still acceptable for partially replacing an entity. But this is why we write:

PUT /wallets/{walletId}/passphrase

and not:

PUT /wallets/{walletId}

{ "passphrase:" ... }

@KtorZ
Copy link
Member Author

KtorZ commented May 11, 2020

bors r+

iohk-bors bot added a commit that referenced this pull request May 11, 2020
1645: Import legacy Byron addresses r=KtorZ a=KtorZ

# Issue Number

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

ADP-292

# Overview

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

- f71d8a8
  📍 **add 'putRandomAddress' to the swagger specifications and API servant definition**
  
- db29bab
  📍 **write dummy server implementation for 'putRandomAddress' and fill the gap in various APIs**
  
- 46aa7c5
  📍 **add integration tests for byron address restoration**
  
- 3f255e7
  📍 **implement server and application handlers for importing an address**
  
- f7ba8fb
  📍 **add command for 'address import' mapping to the newly introduced endpoint**
  
- 2ae81f6
  📍 **add some integration tests scenario for 'address import'**

- 73294ae
  📍 **fix non-atomic checkpoint modification in {import,create}RandomAddress**
    This is subtle but, everytime we are tempted to write:

  ```hs
  cp <- atomically $ readCheckpoint wid
  {- Do some stuff... -}
  atomically $ putCheckpoint wid cp'
  ```

  we should in reality really go for:

  ```hs
  atomically $ do
    cp <- readCheckpoint wid
    {- Do some stuff... -}
    putCheckpoint wid cp'
  ```

  because there's no guarantee that the checkpoint hasn't already
  changed between the moment we fetched it, and the moment we insert a
  modification. This could have disastreous effects like actually
  updating a checkpoint in the past while a new one has already been
  created with now oudated data; or worse, in the case of EBB, it'll
  simply override the newly written checkpoint and makes the database
  silently roll back of exactly 1 block!

# Comments

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

![Screenshot from 2020-05-08 18-05-25](https://user-images.githubusercontent.com/5680256/81424623-89014c00-9156-11ea-9a33-a4e6fb3fcd50.png)

```
$ cardano-wallet-byron address import --help
Usage: cardano-wallet-byron address import [--port INT] WALLET_ID ADDRESS
  Import a random address generated elsewhere. Only available for random
  wallets. The address must belong to the target wallet.

Available options:
  -h,--help                Show this help text
  --port INT               port used for serving the wallet API. (default: 8090)
```

<!-- 
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]>
@KtorZ
Copy link
Member Author

KtorZ commented May 11, 2020

waiting for Hydra again takes ages :(

@KtorZ KtorZ merged commit c99ec25 into master May 11, 2020
@KtorZ KtorZ deleted the KtorZ/ADP-292/import-addresses branch May 11, 2020 11:52
@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 11, 2020

Build failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants