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 verification key endpoint #2229

Merged
merged 15 commits into from
Oct 22, 2020
Merged

add verification key endpoint #2229

merged 15 commits into from
Oct 22, 2020

Conversation

paweljakubas
Copy link
Contributor

@paweljakubas paweljakubas commented Oct 9, 2020 •
edited
Loading

Issue Number

https://jira.iohk.io/browse/ADP-474

Overview

  • I have added endpoint plus basic types and instances
  • I have extended swagger
  • I have updated core unit tests
  • I have added impl and switched on impl in Shelley
  • I have extended Api.Link and core-integration's DSL
  • I have added illustrative integration test
  • I have added golden tests for explicit mnemonic and picked indices based on cardano-addresses

Comments

@paweljakubas paweljakubas self-assigned this Oct 9, 2020
@paweljakubas paweljakubas added the ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG label Oct 9, 2020
rvl
rvl reviewed Oct 11, 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.

Looking good

lib/core/src/Cardano/Wallet/Api/Types.hs Outdated
instance FromHttpApiData ApiRelativeAddressIndex where
parseUrlPiece = bimap (T.pack . getTextDecodingError) ApiRelativeAddressIndex . fromText

instance FromText Word31 where
Copy link
Contributor

Choose a reason for hiding this comment

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

This could go into the text-class package I think, and perhaps it could be defined in terms of another typeclass like Integral.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, although now it is Word32. After the rebase many things need to change

@paweljakubas paweljakubas force-pushed the paweljakubas/adp-474/endpoint-for-ver-keys branch from 1f8047a to e89fc36 Compare October 13, 2020 10:34
@paweljakubas paweljakubas changed the title add basic types and endpoint add verification key endpoint Oct 13, 2020
@paweljakubas paweljakubas force-pushed the paweljakubas/adp-474/endpoint-for-ver-keys branch from 89f06ba to e064518 Compare October 13, 2020 20:12
@paweljakubas paweljakubas requested a review from rvl October 13, 2020 20:13
@paweljakubas paweljakubas marked this pull request as ready for review October 13, 2020 20:14
@paweljakubas
Copy link
Contributor Author

bors try

iohk-bors bot added a commit that referenced this pull request Oct 13, 2020
@iohk-bors
Try #2229:
039fd87
@paweljakubas paweljakubas requested a review from KtorZ October 13, 2020 20:35
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Oct 13, 2020

try

Build failed:

@paweljakubas paweljakubas force-pushed the paweljakubas/adp-474/endpoint-for-ver-keys branch from a4fa67d to 4991183 Compare October 16, 2020 08:11
KtorZ
KtorZ reviewed Oct 16, 2020
View reviewed changes
lib/core/src/Cardano/Wallet/Api.hs Outdated
type GetWalletKey = "wallets"
:> Capture "walletId" (ApiT WalletId)
:> "keys"
:> "script"
Copy link
Member

Choose a reason for hiding this comment

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

What about capturing the full role here, so that we get better re-usability for this endpoint.

KtorZ
KtorZ reviewed Oct 16, 2020
View reviewed changes
lib/core/src/Cardano/Wallet/Api/Server.hs Outdated
let (KeyHash bytes) =
hashKey $ deriveMultisigPublicKey
(liftXPub $ Shelley.getKey accPub)
(toEnum $ fromInteger $ toInteger index)
Copy link
Member

Choose a reason for hiding this comment

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

That logic would better be located in Cardano.Wallet.hs. What about defining a function along those lines:

getWalletKeyHash 
    ::  ctx
    -> WalletId
    -> AccountingStyle
    -> Index 'Soft AddressK
    -> KeyHash

and re-use it in this layer?

lib/core/src/Cardano/Wallet/Api/Types.hs
@@ -711,6 +712,10 @@ data ApiWalletMigrationInfo = ApiWalletMigrationInfo
newtype ApiWithdrawRewards = ApiWithdrawRewards Bool
deriving (Eq, Generic, Show)

newtype ApiVerificationKeyHash = ApiVerificationKeyHash
{ verificationKeyHash :: ApiT (Hash "ScriptKey")
} deriving (Eq, Generic, Show)
Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, we could have the API return a plain bech32-encoded string on success?

lib/text-class/src/Data/Text/Class.hs
@@ -116,6 +118,19 @@ instance FromText Natural where
instance ToText Natural where
toText = T.pack . show

instance FromText Word32 where
fromText =
validate <=< (fmap fromIntegral . fromText @Natural)
Copy link
Member

Choose a reason for hiding this comment

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

This will not work 😱! The fromIntegral that transform from Natural to Word32 needs to happen after the validation to be safe. Otherwise, this may just throw if the value can't be converted back.

Note that @hasufell is doing something similar but re-using constructor from the text package which makes things a bit nicer: https://github.com/input-output-hk/cardano-wallet/pull/2213/files#diff-b892068f732dfe3e67cd6f7a464975ba21a3588de3e5ea74022be5dfaa18e476

I'd suggest cherry-picking this change 👍

KtorZ
KtorZ reviewed Oct 16, 2020
View reviewed changes
stack.yaml Outdated
- git: https://github.com/input-output-hk/cardano-addresses
commit: e0ab4587266430a08734e1aec1c29d261a9a3b70
commit: 02fb4ec7db7c55334ab2216c94cf70b209a33885
Copy link
Member

Choose a reason for hiding this comment

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

We need to use released version of cardano-addresses. There are some cleanup to be done in cardano-addresses before we can release a 3.0.0 version. Let's discuss that on Slack.

@KtorZ KtorZ force-pushed the paweljakubas/adp-474/endpoint-for-ver-keys branch 4 times, most recently from 3d6f3f7 to 8115d73 Compare October 22, 2020 15:38
IOHK and others added 9 commits October 22, 2020 18:15
@KtorZ
Regenerate nix
6bb3b77
@paweljakubas @KtorZ
add getWalletKey to api link
b0b5385
@paweljakubas @KtorZ
extend DSL
d9cf9c8
@paweljakubas @KtorZ
add integration test
7eac373
@paweljakubas @KtorZ
add golden test based on cardano-addresses
f147914
@KtorZ
revert cardano-addresses bump. Sticking to last release for now.
c62a654
@KtorZ
generalize public key derivation in the API for all roles.
cf172b5 
  And also, to keys and not key hashes. The goal of this endpoint is to
  expose public keys so clients can perform signature verification all
  by themselves.
@KtorZ
make sign metadata endpoint consistent with others (move 'signatures'…
fb547f0 
… after wallet id)
@KtorZ
add integration tests for signatures produced by the API
0e10168 
  In particular, we use in conjunction with the other endpoint to get a public key, and show that the public key can verify the signature.
@KtorZ KtorZ force-pushed the paweljakubas/adp-474/endpoint-for-ver-keys branch from 7262598 to 0e10168 Compare October 22, 2020 16:21
@KtorZ
Copy link
Member

KtorZ commented Oct 22, 2020

bors merge

@KtorZ
Copy link
Member

KtorZ commented Oct 22, 2020

bors r-

iohk-bors bot added a commit that referenced this pull request Oct 22, 2020
@iohk-bors @KtorZ @paweljakubas
Merge #2229 #2263 #2264
405a967 
2229: add verification key endpoint r=KtorZ a=paweljakubas

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->
https://jira.iohk.io/browse/ADP-474

# Overview

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

- [x] I have added endpoint plus basic types and instances
- [x] I have extended swagger
- [x] I have updated core unit tests
- [x] I have added impl and switched on impl in Shelley
- [x] I have extended Api.Link and core-integration's DSL
- [x] I have added illustrative integration test
- [x] I have added golden tests for explicit mnemonic and picked indices based on cardano-addresses

# Comments

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

<!-- 
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
-->


2263: Accounting Style Renaming: proper manual migration r=KtorZ a=KtorZ

# Issue Number

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

ADP-523

# Overview

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

- 016f60c
  📍 **reproduce deserialization issue with regards to account style change**
    I originally added a 'backward-compatible' function step to the deserialization function of value of type 'AccountingStyle'. The problem is that it only concerns objects that are being deserialized. Any 'AccountingStyle' value that is queried will inevitably fail because columns in an existing database would still be using the old serialization format. As a consequence, fetching all addresses of an existing sequential wallet returns an empty set, regardless of what is in the database.

- 0a6c36c
  📍 **use a proper manual database migration for renaming AccountingStyle   instead of the hacky in-place adjustment in the deserializer.**

# Comments

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

Whoops...

<!-- 
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
-->


2264: Fix error message on bad query param filtering addresses in Jormungandr tests r=piotr-iohk a=piotr-iohk


# Issue Number

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


# Overview

- 67ef8fb
  Fix error message on bad query param filtering addresses in Jormungandr tests


# Comments

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

<!-- 
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: Pawel Jakubas <[email protected]>
Co-authored-by: IOHK <[email protected]>
Co-authored-by: Piotr Stachyra <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Oct 22, 2020

Canceled.

@KtorZ
Copy link
Member

KtorZ commented Oct 22, 2020

bors merge

iohk-bors bot added a commit that referenced this pull request Oct 22, 2020
@iohk-bors @KtorZ @paweljakubas
Merge #2229 #2263 #2264
c3cde05 
2229: add verification key endpoint r=KtorZ a=paweljakubas

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->
https://jira.iohk.io/browse/ADP-474

# Overview

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

- [x] I have added endpoint plus basic types and instances
- [x] I have extended swagger
- [x] I have updated core unit tests
- [x] I have added impl and switched on impl in Shelley
- [x] I have extended Api.Link and core-integration's DSL
- [x] I have added illustrative integration test
- [x] I have added golden tests for explicit mnemonic and picked indices based on cardano-addresses

# Comments

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

<!-- 
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
-->


2263: Accounting Style Renaming: proper manual migration r=KtorZ a=KtorZ

# Issue Number

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

ADP-523

# Overview

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

- 016f60c
  📍 **reproduce deserialization issue with regards to account style change**
    I originally added a 'backward-compatible' function step to the deserialization function of value of type 'AccountingStyle'. The problem is that it only concerns objects that are being deserialized. Any 'AccountingStyle' value that is queried will inevitably fail because columns in an existing database would still be using the old serialization format. As a consequence, fetching all addresses of an existing sequential wallet returns an empty set, regardless of what is in the database.

- 0a6c36c
  📍 **use a proper manual database migration for renaming AccountingStyle   instead of the hacky in-place adjustment in the deserializer.**

# Comments

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

Whoops...

<!-- 
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
-->


2264: Fix error message on bad query param filtering addresses in Jormungandr tests r=KtorZ a=piotr-iohk


# Issue Number

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


# Overview

- 67ef8fb
  Fix error message on bad query param filtering addresses in Jormungandr tests


# Comments

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

<!-- 
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: Pawel Jakubas <[email protected]>
Co-authored-by: IOHK <[email protected]>
Co-authored-by: Piotr Stachyra <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Oct 22, 2020

This PR was included in a batch that timed out, it will be automatically retried

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Oct 22, 2020

Build succeeded:

@iohk-bors iohk-bors bot merged commit ef1f7ea into master Oct 22, 2020
@iohk-bors iohk-bors bot deleted the paweljakubas/adp-474/endpoint-for-ver-keys branch October 22, 2020 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KtorZ KtorZ KtorZ approved these changes

@rvl rvl Awaiting requested review from rvl

Assignees

@paweljakubas paweljakubas

Labels
ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG
Projects
None yet
Milestone
No milestone
Development

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