-
Notifications
You must be signed in to change notification settings - Fork 214
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
Shared wallets in incomplete state should not undergo restoration #3676
Shared wallets in incomplete state should not undergo restoration #3676
Conversation
104a2d3
to
a00485d
Compare
894456a
to
c6d9676
Compare
5ff2ae6
to
5a01f20
Compare
```patch - createNonrestoringWalletWorker + createNonRestoringWalletWorker ```
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.
Thanks for making this PR!
I've given it a brief look (though still need to look further).
I have one question about the use of terminology:
When you say "no meta", are you referring to a case where the database has no metadata for a given wallet?
@@ -769,6 +769,11 @@ instance IsServerError ErrDerivePublicKey where | |||
instance IsServerError ErrAddCosignerKey where | |||
toServerError = \case | |||
ErrAddCosignerKeyNoSuchWallet e -> toServerError e | |||
ErrAddCosignerKeyNoMeta -> | |||
apiError err503 NoMeta $ mconcat | |||
[ "No meta of wallet was found in database during migration" |
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.
When you refer to "no meta of wallet", do you mean "no wallet metadata"?
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.
yes
This commit makes it clearer that it was not possible to find the **metadata** for a particular wallet. In this particular context, the term "meta" is arguably a bit hard to understand, unless the reader happens to be familiar with the internals of cardano wallet.
This commit revises the error messages relating to `WalletMetadataNotFound`, for readability and flow.
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.
I have played a bit and it seems to work as expected. Also e2e tests pass on the branch.
I think that we could have some integration tests:
- producing the introduced
WalletMetadataNotFound
if that is possible - creating a shared wallet, funding it, then deleting it and creating again "in steps", i.e. create + patch and making sure the balance is as expected.
apiError err503 WalletMetadataNotFound $ T.unwords | ||
[ "It was not possible to find any metadata for the given" | ||
, "wallet within the database. This could be because the" | ||
, "wallet has yet to become active after being in the pending" |
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.
, "wallet has yet to become active after being in the pending" | |
, "wallet has yet to become active after being in the incomplete" |
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.
Q: what are circumstances for this error to occur? Is it possible to produce the test that incurs this error?
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.
if something wrong with db ie. when during being in incomplete db is erased
withWorkerCtx ctx wid liftE liftE $ \wrk -> do | ||
liftHandler $ W.updateCosigner wrk wid (liftKey accXPub) cosigner cred | ||
wal' <- fst <$> getWallet ctx (mkSharedWallet @_ @s @k) (ApiT wid) | ||
-- we switch on restoring only after pending -> active transition |
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.
-- we switch on restoring only after pending -> active transition | |
-- we switch on restoring only after incomplete -> active transition |
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
specifications/api/swagger.yaml
Outdated
wallet within the database. | ||
|
||
May occur when a shared wallet has not yet become active after being in | ||
the pending state. |
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 pending state. | |
the incomplete state. |
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
bors r- restarting |
Canceled. |
bors r+ |
3609: [ADP-2391] implement model API for submissions store r=Anviking a=paolino - [x] implement model API operations for submissions store ### Comments <!-- Additional comments, links, or screenshots to attach, if any. --> ### Issue Number ADP-2391 <!-- Reference the Jira/GitHub issue that this PR relates to, and which requirements it tackles. Note: Jira issues of the form ADP- will be auto-linked. --> 3676: Shared wallets in incomplete state should not undergo restoration r=Anviking a=paweljakubas ### Issue Number ADP-2432 ### Description - [x] I have made sure `Incomplete` shared wallet invokes `createNonRestoringWalletWorker`. - [x] When `Incomplete -> Ready` transition then the private key and meta from incomplete wallet is read. - [x] The incomplete wallet is deleted and active wallet is created. The wallet starts restoring. - [x] Wallet takes meta and private key from incomplete wallet. The solution making db exchange between workers (non-restoring and restoring) did not work properly. hence the above solution. 3680: Shared wallets user guide - creating a wallet and spending r=piotr-iohk a=piotr-iohk - [x] Shared wallets user guide - creating a wallet and spending (fc5615f) ### Comments <!-- Additional comments, links, or screenshots to attach, if any. --> ### Issue Number ADP-2457 3684: Make sure there is collateral on the wallet before e2e plutus tests r=Anviking a=piotr-iohk - [x] Make sure there is collateral on the wallet before e2e plutus tests (925cc09) ### Comments Sometimes wallets turn out to have no pure ADA utxo and plutus tests fail. This is to make sure there are always some pure ADA utxos. #maintenance ### Issue Number n/a Co-authored-by: paolo veronelli <paolo.veronelli@gmail.com> Co-authored-by: Pawel Jakubas <pawel.jakubas@iohk.io> Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io> Co-authored-by: Piotr Stachyra <piotr.stachyra@iohk.io>
Build failed (retrying...): |
3676: Shared wallets in incomplete state should not undergo restoration r=Anviking a=paweljakubas ### Issue Number ADP-2432 ### Description - [x] I have made sure `Incomplete` shared wallet invokes `createNonRestoringWalletWorker`. - [x] When `Incomplete -> Ready` transition then the private key and meta from incomplete wallet is read. - [x] The incomplete wallet is deleted and active wallet is created. The wallet starts restoring. - [x] Wallet takes meta and private key from incomplete wallet. The solution making db exchange between workers (non-restoring and restoring) did not work properly. hence the above solution. 3680: Shared wallets user guide - creating a wallet and spending r=piotr-iohk a=piotr-iohk - [x] Shared wallets user guide - creating a wallet and spending (fc5615f) ### Comments <!-- Additional comments, links, or screenshots to attach, if any. --> ### Issue Number ADP-2457 Co-authored-by: Pawel Jakubas <pawel.jakubas@iohk.io> Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io> Co-authored-by: Piotr Stachyra <piotr.stachyra@iohk.io>
bors r- |
Canceled. |
bors r+ |
3609: [ADP-2391] implement model API for submissions store r=Anviking a=paolino - [x] implement model API operations for submissions store ### Comments <!-- Additional comments, links, or screenshots to attach, if any. --> ### Issue Number ADP-2391 <!-- Reference the Jira/GitHub issue that this PR relates to, and which requirements it tackles. Note: Jira issues of the form ADP- will be auto-linked. --> 3676: Shared wallets in incomplete state should not undergo restoration r=Anviking a=paweljakubas ### Issue Number ADP-2432 ### Description - [x] I have made sure `Incomplete` shared wallet invokes `createNonRestoringWalletWorker`. - [x] When `Incomplete -> Ready` transition then the private key and meta from incomplete wallet is read. - [x] The incomplete wallet is deleted and active wallet is created. The wallet starts restoring. - [x] Wallet takes meta and private key from incomplete wallet. The solution making db exchange between workers (non-restoring and restoring) did not work properly. hence the above solution. 3680: Shared wallets user guide - creating a wallet and spending r=Anviking a=piotr-iohk - [x] Shared wallets user guide - creating a wallet and spending (fc5615f) ### Comments <!-- Additional comments, links, or screenshots to attach, if any. --> ### Issue Number ADP-2457 3684: Make sure there is collateral on the wallet before e2e plutus tests r=Anviking a=piotr-iohk - [x] Make sure there is collateral on the wallet before e2e plutus tests (925cc09) ### Comments Sometimes wallets turn out to have no pure ADA utxo and plutus tests fail. This is to make sure there are always some pure ADA utxos. #maintenance ### Issue Number n/a Co-authored-by: paolo veronelli <paolo.veronelli@gmail.com> Co-authored-by: Pawel Jakubas <pawel.jakubas@iohk.io> Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io> Co-authored-by: Piotr Stachyra <piotr.stachyra@iohk.io>
Build failed (retrying...): |
3676: Shared wallets in incomplete state should not undergo restoration r=Anviking a=paweljakubas ### Issue Number ADP-2432 ### Description - [x] I have made sure `Incomplete` shared wallet invokes `createNonRestoringWalletWorker`. - [x] When `Incomplete -> Ready` transition then the private key and meta from incomplete wallet is read. - [x] The incomplete wallet is deleted and active wallet is created. The wallet starts restoring. - [x] Wallet takes meta and private key from incomplete wallet. The solution making db exchange between workers (non-restoring and restoring) did not work properly. hence the above solution. 3680: Shared wallets user guide - creating a wallet and spending r=Anviking a=piotr-iohk - [x] Shared wallets user guide - creating a wallet and spending (fc5615f) ### Comments <!-- Additional comments, links, or screenshots to attach, if any. --> ### Issue Number ADP-2457 Co-authored-by: Pawel Jakubas <pawel.jakubas@iohk.io> Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io> Co-authored-by: Piotr Stachyra <piotr.stachyra@iohk.io>
This PR was included in a batch that successfully built, but then failed to merge into master. It will not be retried. Additional information: |
bors r+ |
3676: Shared wallets in incomplete state should not undergo restoration r=Anviking a=paweljakubas ### Issue Number ADP-2432 ### Description - [x] I have made sure `Incomplete` shared wallet invokes `createNonRestoringWalletWorker`. - [x] When `Incomplete -> Ready` transition then the private key and meta from incomplete wallet is read. - [x] The incomplete wallet is deleted and active wallet is created. The wallet starts restoring. - [x] Wallet takes meta and private key from incomplete wallet. The solution making db exchange between workers (non-restoring and restoring) did not work properly. hence the above solution. 3680: Shared wallets user guide - creating a wallet and spending r=Anviking a=piotr-iohk - [x] Shared wallets user guide - creating a wallet and spending (fc5615f) ### Comments <!-- Additional comments, links, or screenshots to attach, if any. --> ### Issue Number ADP-2457 Co-authored-by: Pawel Jakubas <pawel.jakubas@iohk.io> Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io> Co-authored-by: Piotr Stachyra <piotr.stachyra@iohk.io>
Build failed (retrying...): |
3676: Shared wallets in incomplete state should not undergo restoration r=Anviking a=paweljakubas ### Issue Number ADP-2432 ### Description - [x] I have made sure `Incomplete` shared wallet invokes `createNonRestoringWalletWorker`. - [x] When `Incomplete -> Ready` transition then the private key and meta from incomplete wallet is read. - [x] The incomplete wallet is deleted and active wallet is created. The wallet starts restoring. - [x] Wallet takes meta and private key from incomplete wallet. The solution making db exchange between workers (non-restoring and restoring) did not work properly. hence the above solution. Co-authored-by: Pawel Jakubas <pawel.jakubas@iohk.io> Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io>
Build failed: |
bors r+ |
Build succeeded: |
… undergo restoration r=Anviking a=paweljakubas ### Issue Number ADP-2432 ### Description - [x] I have made sure `Incomplete` shared wallet invokes `createNonRestoringWalletWorker`. - [x] When `Incomplete -> Ready` transition then the private key and meta from incomplete wallet is read. - [x] The incomplete wallet is deleted and active wallet is created. The wallet starts restoring. - [x] Wallet takes meta and private key from incomplete wallet. The solution making db exchange between workers (non-restoring and restoring) did not work properly. hence the above solution. Co-authored-by: Pawel Jakubas <pawel.jakubas@iohk.io> Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io> Source commit: 24b188a
3689: Use `decodeErrorInfo` in shared wallet integration tests r=jonathanknowles a=jonathanknowles ## Issue Number Follow-on from review of #3676. ## Summary This PR removes a small number of **error string comparisons** from our integration tests, replacing them with equality checks on the API error types themselves. This allows us to remove redundant duplicated error strings from the `Framework.TestData` module, which need to be manually kept in sync with (virtually the same) definitions in the `Cardano.Wallet.Api.Http.Server.Error` module. ## Notes This PR doesn't convert all integration tests to use this style: it's just meant as a demonstration of the overall method. Additionally, many error strings are created through string interpolation. To convert all of our integration tests to use pattern matching on API error types, we'd need to create richer error types similar to that used for the `UtxoTooSmall` error, and remove the use of string interpolation. Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io>
…ation tests r=jonathanknowles a=jonathanknowles ## Issue Number Follow-on from review of #3676. ## Summary This PR removes a small number of **error string comparisons** from our integration tests, replacing them with equality checks on the API error types themselves. This allows us to remove redundant duplicated error strings from the `Framework.TestData` module, which need to be manually kept in sync with (virtually the same) definitions in the `Cardano.Wallet.Api.Http.Server.Error` module. ## Notes This PR doesn't convert all integration tests to use this style: it's just meant as a demonstration of the overall method. Additionally, many error strings are created through string interpolation. To convert all of our integration tests to use pattern matching on API error types, we'd need to create richer error types similar to that used for the `UtxoTooSmall` error, and remove the use of string interpolation. Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io> Source commit: eba2723
3693: Machine-readable shared wallet API errors. r=jonathanknowles a=jonathanknowles ## Issue Number Follow-on from review of #3676. ## Summary This PR removes an **interpolated string equality check** from our API integration test suite, and replaces it an equality check that uses **`decodeErrorInfo`** in conjunction with an **API error record**: ```patch lib/wallet/integration/src/Test/Integration/Scenario/API/Shared/Wallets.hs - expectErrorMessage (errMsg403NoSuchCosigner (toText Payment) 7) rPatch5 + decodeErrorInfo rPatch5 `shouldBe` SharedWalletNoSuchCosigner + ApiErrorSharedWalletNoSuchCosigner + { cosignerIndex = ApiCosignerIndex 7 + , credentialType = ApiCredentialType Payment + } ``` ```patch lib/wallet/integration/src/Test/Integration/Framework/TestData.hs - errMsg403NoSuchCosigner :: Text -> Int -> String - errMsg403NoSuchCosigner cred cosigner = unwords - [ "It looks like you've tried to add a cosigner key to a" - , "shared wallet's" - , unpack cred - , "template for a non-existing cosigner index:" - , show cosigner - ] ``` The above definition of `errMsg403NoCosigner` (now removed) was essentially a **duplicate** of an identical interpolated string in `Http.Server.Error`: https://github.com/input-output-hk/cardano-wallet/blob/1263e6ed8c486f31ed75219b59a8e5fc6af577de/lib/wallet/api/http/Cardano/Wallet/Api/Http/Server/Error.hs#L801-L807 ## Advantages 1. If we make any breaking changes to the API error type, the compiler will warn us to update the test suite. 2. Someone browsing the integration tests can easily jump to the definition of the error type, or to the definitions of any of the constructors used. 3. We no longer need to manually keep two copies of an interpolated string function in sync with one another. ## Method To achieve this, we introduce the following structured error type: ```haskell data ApiErrorSharedWalletNoSuchCosigner = ApiErrorSharedWalletNoSuchCosigner { cosignerIndex :: !ApiCosignerIndex , credentialType :: !ApiCredentialType } deriving (Data, Eq, Generic, Show, Typeable) deriving (FromJSON, ToJSON) via DefaultRecord ApiErrorSharedWalletNoSuchCosigner deriving anyclass NFData ``` And add it as a parameter to the relevant constructor of ApiErrorInfo: ```patch lib/wallet/api/http/Cardano/Wallet/Api/Types/Error.hs data ApiErrorInfo = ... | ... - | SharedWalletNoSuchCosigner + | SharedWalletNoSuchCosigner !ApiErrorSharedWalletNoSuchCosigner | ... ``` When errors of this type are encoded as JSON, this results in objects of the following format: ```json { "code": "shared_wallet_no_such_cosigner" , "info": { "cosigner_index": 182 , "credential_type": "delegation" } , "message": "It looks like you've..." } ``` ## Future Work This PR serves as a demonstration of this technique, and only converts one particular error type. We could use this method to remove all duplicate interpolated error messages from our API integration test suite. Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io>
3693: Machine-readable shared wallet API errors. r=jonathanknowles a=jonathanknowles ## Issue Number Follow-on from review of #3676. ## Summary This PR removes an **interpolated string equality check** from our API integration test suite, and replaces it an equality check that uses **`decodeErrorInfo`** in conjunction with an **API error record**: ```patch lib/wallet/integration/src/Test/Integration/Scenario/API/Shared/Wallets.hs - expectErrorMessage (errMsg403NoSuchCosigner (toText Payment) 7) rPatch5 + decodeErrorInfo rPatch5 `shouldBe` SharedWalletNoSuchCosigner + ApiErrorSharedWalletNoSuchCosigner + { cosignerIndex = ApiCosignerIndex 7 + , credentialType = ApiCredentialType Payment + } ``` ```patch lib/wallet/integration/src/Test/Integration/Framework/TestData.hs - errMsg403NoSuchCosigner :: Text -> Int -> String - errMsg403NoSuchCosigner cred cosigner = unwords - [ "It looks like you've tried to add a cosigner key to a" - , "shared wallet's" - , unpack cred - , "template for a non-existing cosigner index:" - , show cosigner - ] ``` The above definition of `errMsg403NoCosigner` (now removed) was essentially a **duplicate** of an identical interpolated string in `Http.Server.Error`: https://github.com/input-output-hk/cardano-wallet/blob/1263e6ed8c486f31ed75219b59a8e5fc6af577de/lib/wallet/api/http/Cardano/Wallet/Api/Http/Server/Error.hs#L801-L807 ## Advantages 1. If we make any breaking changes to the API error type, the compiler will warn us to update the test suite. 2. Someone browsing the integration tests can easily jump to the definition of the error type, or to the definitions of any of the constructors used. 3. We no longer need to manually keep two copies of an interpolated string function in sync with one another. ## Method To achieve this, we introduce the following structured error type: ```haskell data ApiErrorSharedWalletNoSuchCosigner = ApiErrorSharedWalletNoSuchCosigner { cosignerIndex :: !ApiCosignerIndex , credentialType :: !ApiCredentialType } deriving (Data, Eq, Generic, Show, Typeable) deriving (FromJSON, ToJSON) via DefaultRecord ApiErrorSharedWalletNoSuchCosigner deriving anyclass NFData ``` And add it as a parameter to the relevant constructor of ApiErrorInfo: ```patch lib/wallet/api/http/Cardano/Wallet/Api/Types/Error.hs data ApiErrorInfo = ... | ... - | SharedWalletNoSuchCosigner + | SharedWalletNoSuchCosigner !ApiErrorSharedWalletNoSuchCosigner | ... ``` When errors of this type are encoded as JSON, this results in objects of the following format: ```json { "code": "shared_wallet_no_such_cosigner" , "info": { "cosigner_index": 182 , "credential_type": "delegation" } , "message": "It looks like you've..." } ``` ## Future Work This PR serves as a demonstration of this technique, and only converts one particular error type. We could use this method to remove all duplicate interpolated error messages from our API integration test suite. Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io>
…jonathanknowles a=jonathanknowles ## Issue Number Follow-on from review of #3676. ## Summary This PR removes an **interpolated string equality check** from our API integration test suite, and replaces it an equality check that uses **`decodeErrorInfo`** in conjunction with an **API error record**: ```patch lib/wallet/integration/src/Test/Integration/Scenario/API/Shared/Wallets.hs - expectErrorMessage (errMsg403NoSuchCosigner (toText Payment) 7) rPatch5 + decodeErrorInfo rPatch5 `shouldBe` SharedWalletNoSuchCosigner + ApiErrorSharedWalletNoSuchCosigner + { cosignerIndex = ApiCosignerIndex 7 + , credentialType = ApiCredentialType Payment + } ``` ```patch lib/wallet/integration/src/Test/Integration/Framework/TestData.hs - errMsg403NoSuchCosigner :: Text -> Int -> String - errMsg403NoSuchCosigner cred cosigner = unwords - [ "It looks like you've tried to add a cosigner key to a" - , "shared wallet's" - , unpack cred - , "template for a non-existing cosigner index:" - , show cosigner - ] ``` The above definition of `errMsg403NoCosigner` (now removed) was essentially a **duplicate** of an identical interpolated string in `Http.Server.Error`: https://github.com/input-output-hk/cardano-wallet/blob/1263e6ed8c486f31ed75219b59a8e5fc6af577de/lib/wallet/api/http/Cardano/Wallet/Api/Http/Server/Error.hs#L801-L807 ## Advantages 1. If we make any breaking changes to the API error type, the compiler will warn us to update the test suite. 2. Someone browsing the integration tests can easily jump to the definition of the error type, or to the definitions of any of the constructors used. 3. We no longer need to manually keep two copies of an interpolated string function in sync with one another. ## Method To achieve this, we introduce the following structured error type: ```haskell data ApiErrorSharedWalletNoSuchCosigner = ApiErrorSharedWalletNoSuchCosigner { cosignerIndex :: !ApiCosignerIndex , credentialType :: !ApiCredentialType } deriving (Data, Eq, Generic, Show, Typeable) deriving (FromJSON, ToJSON) via DefaultRecord ApiErrorSharedWalletNoSuchCosigner deriving anyclass NFData ``` And add it as a parameter to the relevant constructor of ApiErrorInfo: ```patch lib/wallet/api/http/Cardano/Wallet/Api/Types/Error.hs data ApiErrorInfo = ... | ... - | SharedWalletNoSuchCosigner + | SharedWalletNoSuchCosigner !ApiErrorSharedWalletNoSuchCosigner | ... ``` When errors of this type are encoded as JSON, this results in objects of the following format: ```json { "code": "shared_wallet_no_such_cosigner" , "info": { "cosigner_index": 182 , "credential_type": "delegation" } , "message": "It looks like you've..." } ``` ## Future Work This PR serves as a demonstration of this technique, and only converts one particular error type. We could use this method to remove all duplicate interpolated error messages from our API integration test suite. Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io> Source commit: 76fdaf9
Issue Number
ADP-2432
Description
Incomplete
shared wallet invokescreateNonRestoringWalletWorker
.Incomplete -> Ready
transition then the private key and meta from incomplete wallet is read.The solution making db exchange between workers (non-restoring and restoring) did not work properly. hence the above solution.