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

Example off BTCPay API passing unnecessarily degraded and misleading error back #6261

Closed
johnheenan opened this issue Oct 1, 2024 · 4 comments · Fixed by #6393
Closed

Example off BTCPay API passing unnecessarily degraded and misleading error back #6261

johnheenan opened this issue Oct 1, 2024 · 4 comments · Fixed by #6393
Labels
API Questions and issues related to API

Comments

@johnheenan
Copy link

johnheenan commented Oct 1, 2024

Please see #6248 for an example of API passing unnecessarily degraded and misleading error back.

The issue was solved by OP of the issue by removing seed entry wallet and reentering it with 'hot wallet' enabled.

The solution could have been more easily determined by returning more specific error information that was available but ignored.

@johnheenan johnheenan changed the title Example off BTCPay plugin passing unnecessarily degraded and misleading error back Example off BTCPay API passing unnecessarily degraded and misleading error back Oct 1, 2024
@pavlenex
Copy link
Contributor

pavlenex commented Oct 1, 2024

Hi, is this a duplicate of #6263? I don't understand why you've opened a new issue. If not please don't ignore our issue templates, provide more context, and I'll reopen it.

@pavlenex pavlenex closed this as not planned Won't fix, can't repro, duplicate, stale Oct 1, 2024
@johnheenan
Copy link
Author

johnheenan commented Oct 1, 2024

Hi, is this a duplicate of #6263?

No . The #6263 issue above is solely a UI issue.

This issue that you have just closed is solely about unnecessarily degraded, deficient and misleading information passed back from the API with nothing to do with UI.

Both of these are clear in the title.

Both reference the same issue, #6248, for context, that was properly raised according to the context the OP had with the degraded information they were provided with. While this is not relevant here, there was good reason to believe a hot wallet was in use following an answer to a question I posed on chat.btcpayserver.org. It is incredibly easy to get confused about what workflows were used when. Providing confused degraded information in API error messages helps no one.

What is relevant is that I added specific links to source code in the referenced issue and it should not be necessary for me to provide them again here because context is lost. The fact that it is the same issue now used for context for two other issues has nothing to do with two completely separate issues now properly raised.

As pointed out in the other raised issue, the OP solved the problem by removing seed phrases and reinserting it with 'hot wallet' toggle on. However this is not relevant to observing how degraded the error was for this issue.

Please reopen until satisfactorily dealt with.

Some of my comments from issue #6248 follow for the record. However it is better to view them from the context of the issue they were raised in.

For v1.13.5, the error occurs due to network.ReadonlyWallet evaluating to true. network is an out var due to call to IsInvalidWalletRequest with cryptoCode as the only in var. For the file below, no other endpoint function reads network.ReadonlyWallet.

            if (network.ReadonlyWallet)
            {
                return this.CreateAPIError(503, "not-available",
                    $"{cryptoCode} sending services are not currently available");
            }

Above matches the errors reported freom the POST endpoint.

There is a second part of API POST function where the same error message can result

if (!derivationScheme.IsHotWallet || signingKeyStr is null)

          if (!derivationScheme.IsHotWallet || signingKeyStr is null)
            {
                return this.CreateAPIError(503, "not-available",
                    $"{cryptoCode} sending services are not currently available");
            }

Again, the wallet is 'hot' because seed words were used. Also the wallet can be used for transactions through UI.

derivationScheme also comes from IsInvalidWalletRequest function as an out var. Also this function can throw the same error message

@johnheenan
Copy link
Author

Pinging @pavlenex to get this issue reopened

@pavlenex pavlenex reopened this Oct 1, 2024
@pavlenex pavlenex added the API Questions and issues related to API label Nov 12, 2024
@NicolasDorier
Copy link
Member

I will improve error message

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Questions and issues related to API
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants