-
Notifications
You must be signed in to change notification settings - Fork 220
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
Improve shared wallets #2663
Improve shared wallets #2663
Conversation
4be0960
to
4e11b5e
Compare
c057c29
to
2ed0a9d
Compare
b8e8ffb
to
9f93930
Compare
9291322
to
e4a99de
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Nice improvements. I have some suggestions though.
, expectField #format (`shouldBe` Extended) | ||
] | ||
let (ApiAccountKeyShared bytes' _) = getFromResponse id aKey | ||
T.decodeUtf8 (hex bytes') `Expectations.shouldBe` accXPubTxt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lifted expectations module is meant to replace Test.Hspec - so no need to import it qualified.
Also a util function for hexText would be nice.
lib/core/src/Cardano/Wallet.hs
Outdated
@@ -2391,8 +2412,8 @@ data ErrAddCosignerKey | |||
deriving (Eq, Show) | |||
|
|||
data ErrConstructSharedWallet | |||
= ErrConstructSharedWalletMissingKey | |||
-- ^ The shared wallet' script template doesn't have the wallet's account public key | |||
= ErrConstructSharedWalletWrongScriptTemplate CredentialType Text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we put this error type into the AddressDiscovery.Shared
module with a ToText
instance, and Text
replaced with ErrValidateScriptTemplate
?
lib/core/src/Cardano/Wallet/Api.hs
Outdated
type GetAccountKey = "wallets" | ||
:> Capture "walletId" (ApiT WalletId) | ||
:> "keys" | ||
:> QueryParam "extended" Bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably OK to have hash
as a bool query param, but maybe not extended
. Didn't we use a sum type elsewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
introduced "format=extended" and "format=non_extended" and used Maybe KeyFormat
rather than Maybe Bool
xPubtoBytes :: KeyFormat -> XPub -> ByteString | ||
xPubtoBytes = \case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was confusing with the letter case differences. How about:
xPubtoBytes :: KeyFormat -> XPub -> ByteString | |
xPubtoBytes = \case | |
publicKeyToBytes' :: WalletKey k => KeyFormat -> k -> ByteString | |
publickKeyToBytes' f k = case f of | |
Extended -> xpubToBytes $ getRawKey k | |
NonExtended -> xpubPublicKey $ getRawKey k |
parseRole = \case | ||
hrp | hrp == [humanReadablePart|addr_vk|] -> pure UtxoExternal | ||
hrp | hrp == [humanReadablePart|stake_vk|] -> pure MutableAccount | ||
parseRoleHashing = \case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would it look if we get the hrp as text, split on "_"
and pattern match on that?
specifications/api/swagger.yaml
Outdated
Returned when a user tries to create a shared wallet with script template | ||
that is not validated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returned when a user tries to create a shared wallet with script template | |
that is not validated. | |
Returned when a user tries to create a shared wallet with script template | |
that does not pass validation. |
specifications/api/swagger.yaml
Outdated
code: | ||
type: string | ||
enum: ['shared_wallet_create_not_allowed'] | ||
enum: ['shared_wallet_script_template_not_validated'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not validated = Invalid?
enum: ['shared_wallet_script_template_not_validated'] | |
enum: ['shared_wallet_script_template_invalid'] |
specifications/api/swagger.yaml
Outdated
@@ -4505,7 +4534,7 @@ paths: | |||
summary: Create | |||
description: | | |||
<p align="right">status: <strong>stable</strong></p> | |||
Retrieve account public key from the wallet. | |||
Retrieve any account public key from the wallet provided it was created from mnemonics. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Retrieve any account public key from the wallet provided it was created from mnemonics. | |
Derive an account public key for any account index. For this key derivation to be possible, the wallet must have been created from mnemonic. |
specifications/api/swagger.yaml
Outdated
@@ -5441,7 +5489,7 @@ paths: | |||
summary: Create | |||
description: | | |||
<p align="right">status: <strong>stable</strong></p> | |||
Retrieve account public key from the shared wallet. | |||
Derive any account public key from the shared wallet provided the wallet was created from mnemonics. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment as above
specifications/api/swagger.yaml
Outdated
summary: Get | ||
description: | | ||
<p align="right">status: <strong>stable</strong></p> | ||
Retrieve account public key of the shared wallet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Retrieve account public key of the shared wallet. | |
Retrieve the account public key of this shared wallet. |
e4a99de
to
cc5f4b0
Compare
@paweljakubas perhaps that's a silly question, but why do we need two separate endpoints for getting account public key? Couldn't this be realized by already existing:
I assume this was done to address:
The same behavior is for shelley wallets. Both have been considered as a bug as per comment -> #2624 (comment) |
specifications/api/swagger.yaml
Outdated
properties: | ||
id: *walletId | ||
name: *walletName | ||
account_index: *derivationSegment | ||
address_pool_gap: *walletAddressPoolGap | ||
payment_script_template: *scriptTemplate | ||
delegation_script_template: *scriptTemplate | ||
status: *statusIncomplete |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I see the status of complete/active wallet is displayed as:
"state": {
"status": "syncing",
...
}
or
"state": {
"status": "ready"
}
Whereas incomplete wallet's status is not wrapped into state
and displays only:
"status": "incomplete"
Maybe it would be nice to keep this convention and have:
"state": {
"status": "incomplete"
}
(That's something I'd expect as an API user at least)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@piotr-iohk I was thinking long how to fix "posting" account public keys for account based wallets. And get to the conclusion that we should have POST which stands for "deriving" and it means we need mnemonics to do it, so the wallet have to be root key based. Also we can GET the account public key for a given wallet, and it should work for each wallet, account and root based, and also for shared wallets and shelley wallets. I added support for both shelley and shared wallets, also checked in integration testing that it works for root and account based wallets |
bors r+ |
bors r- |
Canceled. |
bors r+ |
2663: Improve shared wallets r=paweljakubas a=paweljakubas # Issue Number <!-- Put here a reference to the issue that this PR relates to and which requirements it tackles. Jira issues of the form ADP- will be auto-linked. --> adp-934 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - [x] Adding "incomplete" to status for pending shared wallets - [x] comprehensive validation of script templates when posting them - [x] integration testing of validation of script templates - [x] add GET /shared-wallets/wid/keys and whole infrastructure - [x] add GET /wallets/wid/keys - [x] unite ApiVerificationKeyShelley and ApiVerificationKeyShared and hence enable with/without hashing capability for shelley style - [x] better reuse of code in Api.Link - [x] test new endpoints # Comments Prerequisite : IntersectMBO/cardano-addresses#131 <!-- 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) ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket ✓ Acknowledge any changes required to the Wiki ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages. --> 2667: Add property test for `UTxOIndex.selectRandomWithPriority`. r=jonathanknowles a=jonathanknowles # Issue Number ADP-890 # Overview This PR adds a property test for `UTxOIndex.selectRandomWithPriority`. The `selectRandomWithPriority` function is designed to: - select an entry at random from a UTxO index according to a specified list of filter conditions; - traverse the specified list of filter conditions in order of priority **_from left to right_**. The test added in this PR provides a basic sanity check to verify that priority order is respected. # Sample Output ```hs Cardano.Wallet.Primitive.Types.UTxOIndex Indexed UTxO set properties Index Selection prop_selectRandomWithPriority +++ OK, passed 1600 tests: 59.69% have match for neither asset 1 nor asset 2 17.12% have match for asset 1 but not for asset 2 16.31% have match for asset 2 but not for asset 1 6.88% have match for both asset 1 and asset 2 Finished in 1.0870 seconds 1 example, 0 failures ``` # QA Due Diligence I ran this test 500 times to increase confidence that it will not fail spuriously. No failures were encountered. Co-authored-by: Pawel Jakubas <[email protected]> Co-authored-by: IOHK <[email protected]> Co-authored-by: Rodney Lorrimar <[email protected]> Co-authored-by: Jonathan Knowles <[email protected]>
Build failed (retrying...): #expected |
2663: Improve shared wallets r=paweljakubas a=paweljakubas # Issue Number <!-- Put here a reference to the issue that this PR relates to and which requirements it tackles. Jira issues of the form ADP- will be auto-linked. --> adp-934 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - [x] Adding "incomplete" to status for pending shared wallets - [x] comprehensive validation of script templates when posting them - [x] integration testing of validation of script templates - [x] add GET /shared-wallets/wid/keys and whole infrastructure - [x] add GET /wallets/wid/keys - [x] unite ApiVerificationKeyShelley and ApiVerificationKeyShared and hence enable with/without hashing capability for shelley style - [x] better reuse of code in Api.Link - [x] test new endpoints # Comments Prerequisite : IntersectMBO/cardano-addresses#131 <!-- 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) ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket ✓ Acknowledge any changes required to the Wiki ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages. --> Co-authored-by: Pawel Jakubas <[email protected]> Co-authored-by: IOHK <[email protected]> Co-authored-by: Rodney Lorrimar <[email protected]>
Build failed:
These failures look legit. |
@piotr-iohk Thanks for the question about multiple API endpoints which do similar things. This is not a silly question at all and it's good to take a step back and assess the API as a whole, like you have done. To add to @paweljakubas answer, yes they do slightly different things. The POST one can derive an account public key for any index. To do this, it must be supplied with the password to decrypt the root key. The GET one can only return the "default" or "current" account public key for this wallet. That key is already cached in the wallet state, so no need to derive anything, and therefore no need for a password. Then we get on to the mechanics of HTTP. It is generally discouraged to put passwords in GET query parameters. The preferred way is to use a HTTP header or put the password into the request body of a POST request (the latter often being easier for API users). However, these key derivation endpoints are really GETs, semantically speaking. Anyway, your comment convinced me that it is less confusing to have only one account key endpoint, with optional parameters. It has to be POST. After this PR, I would like to spec out a streamlined API for deriving, which will save us work in future. |
Reviewed-by: Rodney Lorrimar <[email protected]>
The order of function parameters was wrong.
7bda368
to
a74d89c
Compare
I squashed the git history a bit, rebased, made a minor stylistic change to bors r+ |
@rvl @paweljakubas thanks for your responses.
@rvl is this something we'd like to tackle as part of ADP-934 or later? |
I think it can be a separate ticket, so we don't hold up the address derivation part of multisig. |
Build succeeded: |
2663: Improve shared wallets r=rvl a=paweljakubas # Issue Number <!-- Put here a reference to the issue that this PR relates to and which requirements it tackles. Jira issues of the form ADP- will be auto-linked. --> adp-934 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - [x] Adding "incomplete" to status for pending shared wallets - [x] comprehensive validation of script templates when posting them - [x] integration testing of validation of script templates - [x] add GET /shared-wallets/wid/keys and whole infrastructure - [x] add GET /wallets/wid/keys - [x] unite ApiVerificationKeyShelley and ApiVerificationKeyShared and hence enable with/without hashing capability for shelley style - [x] better reuse of code in Api.Link - [x] test new endpoints # Comments Prerequisite : IntersectMBO/cardano-addresses#131 <!-- 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) ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket ✓ Acknowledge any changes required to the Wiki ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages. --> Co-authored-by: Pawel Jakubas <[email protected]> Co-authored-by: Rodney Lorrimar <[email protected]> Co-authored-by: IOHK <[email protected]> 1e89c9a
Issue Number
adp-934
Overview
Comments
Prerequisite : IntersectMBO/cardano-addresses#131