[ADP-3306] Make `NoSuchWallet` error machine-readable. #4597

paweljakubas · 2024-05-17T14:44:43Z

Extend NoSuchWallet to include WalletId.
Adjust unit tests and swagger accordingly.
use NoSuchWallet rather than err403NoSuchErrorMsg throughout integration tests

Issue

ADP-3306

jonathanknowles

Hi @paweljakubas

Thanks for making this PR! I've left a few suggestions.

I believe the unit test failure will be fixed by: #4598.

One last request: would it be possible to fill out the PR description?

Many thanks again!

lib/integration/scenarios/Test/Integration/Scenario/API/Shelley/Transactions.hs

lib/api/src/Cardano/Wallet/Api/Types/Error.hs

jonathanknowles · 2024-05-20T09:14:40Z

lib/api/src/Cardano/Wallet/Api/Types/Error.hs

@@ -312,3 +315,10 @@ data ApiErrorNoSuchPool = ApiErrorNoSuchPool
    deriving (Data, Eq, Generic, Show, Typeable)
    deriving (FromJSON, ToJSON) via DefaultRecord ApiErrorNoSuchPool
    deriving anyclass NFData
+
+data ApiErrorNoSuchWallet = ApiErrorNoSuchWallet
+    { walletId :: !Text


Suggestion: does it make sense to use ApiT WalletId here?

Suggested change

{ walletId :: !Text

{ walletId :: !(ApiT WalletId)

trying to use pattern synonyms here -> 984bb6e

jonathanknowles · 2024-05-20T09:17:35Z

lib/integration/scenarios/Test/Integration/Scenario/API/Byron/Addresses.hs

        ]
+    decodeErrorInfo r `Lifted.shouldBe`
+        (NoSuchWallet $ ApiErrorNoSuchWallet $ w ^. walletId)


For cases like this, I would suggest:

Suggested change

(NoSuchWallet $ ApiErrorNoSuchWallet $ w ^. walletId)

NoSuchWallet (ApiErrorNoSuchWallet $ w ^. walletId)

(I think hlint highlights a series of similar cases.)

indeed, done

@paweljakubas hlint is also failing for these: https://buildkite.com/cardano-foundation/cardano-wallet/builds/4956#018f8702-7a5f-465b-8b49-1b376461a17a

jonathanknowles · 2024-05-20T09:18:52Z

specifications/api/swagger.yaml

@@ -4361,6 +4361,18 @@ x-errNoUtxosAvailable: &errNoUtxosAvailable
      type: string
      enum: ['no_utxos_available']

+x-errNoSuchWalletInitialized: &errNoSuchWalletInitialized


Related to #4597 (comment) (above):

Suggested change

x-errNoSuchWalletInitialized: &errNoSuchWalletInitialized

x-errWalletNotInitialized: &errWalletNotInitialized

jonathanknowles · 2024-05-20T09:19:05Z

specifications/api/swagger.yaml

@@ -4361,6 +4361,18 @@ x-errNoUtxosAvailable: &errNoUtxosAvailable
      type: string
      enum: ['no_utxos_available']

+x-errNoSuchWalletInitialized: &errNoSuchWalletInitialized
+  <<: *responsesErr
+  title: no_such_wallet_initialized


Related to #4597 (comment) (above):

Suggested change

title: no_such_wallet_initialized

title: wallet_not_initialized

specifications/api/swagger.yaml

jonathanknowles · 2024-05-20T09:20:16Z

specifications/api/swagger.yaml

@@ -5627,6 +5645,13 @@ x-responsesErr404WalletNotFound: &responsesErr404WalletNotFound
      application/json:
        schema: *errNoSuchWallet

+x-responsesErr404DbNotInitialized: &responsesErr404DbNotInitialized
+  404:
+    description: DB uninitialized


Suggested change

description: DB uninitialized

description: Wallet not yet initialized.

…SchemaApiErrorInfo`. (#4598) This PR makes a small fix for #4597. ## Issue ADP-3306

fix

This is necessary, as type `ApiErrorNoSuchWallet` uses a real `WalletId` instead of just `Text`.

jonathanknowles

LGTM!

@paweljakubas, many thanks for making this PR!

This particular test happens to use `rawRequest` instead of `request`. Whereas the `request` function attempts to decode the response into a JSON `Value`, the `rawRequest` function does not perform any decoding. This causes the subsequent `decodeErrorInfo` to fail with: ``` uncaught exception: ErrorCall decodeErrorInfo: Expected a 'ClientError', but encountered something else: Left (RawClientError "{\"code\":\"no_such_wallet\",\"info\":{\"wallet_id\":\"acab1951ad1ef5808eb9756b02d1ff85caf0e702\"},\"message\":\"I couldn't find a wallet with the given id: acab1951ad1ef5808eb9756b02d1ff85caf0e702\"}") CallStack (from HasCallStack): error, called at framework/Test/Integration/Framework/DSL.hs:742:13 in cardano-wallet-integration-2024.5.5-KhWmuWPIqBp7pTemTBE3I2-framework:Test.Integration.Framework.DSL decodeErrorInfo, called at scenarios/Test/Integration/Scenario/API/Shelley/Wallets.hs:1495:9 in cardano-wallet-integration-2024.5.5-AcHGWrd1jBGKoKhY01VJRG-scenarios:Test.Integration.Scenario.API.Shelley.Wallets ```

…- [x] Extend `NoSuchWallet` to include `WalletId`. - [x] Adjust unit tests and swagger accordingly. - [x] use `NoSuchWallet` rather than `err403NoSuchErrorMsg` throughout integration tests ### Issue ADP-3306 Source commit: 8faf0a2

paweljakubas requested a review from jonathanknowles May 17, 2024 14:44

paweljakubas self-assigned this May 17, 2024

jonathanknowles mentioned this pull request May 20, 2024

[ADP-3306] Fix call to dataTypeConstrs in FromJSON instance for SchemaApiErrorInfo. #4598

Merged

jonathanknowles reviewed May 20, 2024

View reviewed changes

jonathanknowles changed the title ~~Refactor no such wallet error~~ [ADP-3306] Refactor NoSuchWallet error. May 20, 2024

paweljakubas added a commit that referenced this pull request May 20, 2024

[ADP-3306] Fix call to dataTypeConstrs in FromJSON instance for `…

ee5c073

…SchemaApiErrorInfo`. (#4598) This PR makes a small fix for #4597. ## Issue ADP-3306

jonathanknowles changed the title ~~[ADP-3306] Refactor NoSuchWallet error.~~ [ADP-3306] Make NoSuchWallet error machine-readable. May 22, 2024

paweljakubas added 19 commits May 22, 2024 13:59

introduce ApiErrorNoSuchWallet error

62696fd

swagger

47379d0

remove err403NoSuchWallet - part 1

b76246c

fix

refactor Shared.Wallet

8bc8161

refactor Shelley.StakePools

cd2c47c

refactor Shared.Transactions

0977b1a

refactor Shelley.TransactionsNew

5798a13

fix

Shelley.Addresses

48bfe95

refactor Byron.Migrations

fd55950

Shelley.Migrations

3b2c25e

refactor Shelley.Transaction

d7a72a4

sort out DSL and ApiError naming overlap

b5bd885

Shelley.Wallets

fca2c64

Byron.Transactions

607903e

Byron.Wallets

4196e24

Byron.Addresses

1b40fa3

Byron.CoinSelections

60f1249

revert CLI changes

6557962

reqquested review changes

64dc4ab

paweljakubas force-pushed the paweljakubas/adp-3306/refactor-no-such-wallet branch from 9dc071b to 984bb6e Compare May 22, 2024 15:12

paweljakubas requested a review from jonathanknowles May 22, 2024 15:13

jonathanknowles added 2 commits May 23, 2024 06:37

Make WalletId an instance of Data.

c664feb

Use ApiT WalletId instead of Text for ApiErrorNoSuchWallet.

00fd690

Regenerate ApiErrorNoSuchWallet.json.

c9a339a

This is necessary, as type `ApiErrorNoSuchWallet` uses a real `WalletId` instead of just `Text`.

jonathanknowles force-pushed the paweljakubas/adp-3306/refactor-no-such-wallet branch from 984bb6e to c9a339a Compare May 23, 2024 06:45

Rename Err404DbNotInitialized to Err404WalletNotInitialized.

0f2d84a

jonathanknowles approved these changes May 23, 2024

View reviewed changes

jonathanknowles added 2 commits May 23, 2024 16:00

Merge branch 'master' into paweljakubas/adp-3306/refactor-no-such-wallet

ae50531

Fix order of imports, placed out of order by prior merge.

5253bc6

jonathanknowles enabled auto-merge May 23, 2024 08:37

jonathanknowles added this pull request to the merge queue May 23, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 23, 2024

jonathanknowles added this pull request to the merge queue May 23, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 23, 2024

paweljakubas added this pull request to the merge queue May 23, 2024

Merged via the queue into master with commit 8faf0a2 May 23, 2024
3 checks passed

paweljakubas deleted the paweljakubas/adp-3306/refactor-no-such-wallet branch May 23, 2024 12:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ADP-3306] Make `NoSuchWallet` error machine-readable. #4597

[ADP-3306] Make `NoSuchWallet` error machine-readable. #4597

paweljakubas commented May 17, 2024 •

edited by jonathanknowles

Loading

jonathanknowles left a comment •

edited

Loading

jonathanknowles May 20, 2024

paweljakubas May 22, 2024

jonathanknowles May 20, 2024

jonathanknowles May 20, 2024

paweljakubas May 22, 2024

jonathanknowles May 22, 2024

jonathanknowles May 20, 2024 •

edited

Loading

paweljakubas May 22, 2024

jonathanknowles May 20, 2024 •

edited

Loading

jonathanknowles May 20, 2024

jonathanknowles left a comment

	(NoSuchWallet $ ApiErrorNoSuchWallet $ w ^. walletId)
	NoSuchWallet (ApiErrorNoSuchWallet $ w ^. walletId)

	x-errNoSuchWalletInitialized: &errNoSuchWalletInitialized
	x-errWalletNotInitialized: &errWalletNotInitialized

	title: no_such_wallet_initialized
	title: wallet_not_initialized

	description: DB uninitialized
	description: Wallet not yet initialized.

[ADP-3306] Make NoSuchWallet error machine-readable. #4597

[ADP-3306] Make NoSuchWallet error machine-readable. #4597

Conversation

paweljakubas commented May 17, 2024 • edited by jonathanknowles Loading

Issue

jonathanknowles left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanknowles May 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanknowles May 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanknowles left a comment

Choose a reason for hiding this comment

[ADP-3306] Make `NoSuchWallet` error machine-readable. #4597

[ADP-3306] Make `NoSuchWallet` error machine-readable. #4597

paweljakubas commented May 17, 2024 •

edited by jonathanknowles

Loading

jonathanknowles left a comment •

edited

Loading

jonathanknowles May 20, 2024 •

edited

Loading

jonathanknowles May 20, 2024 •

edited

Loading