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

chore: return lnurlp status and error message when no data can be retrieved #8

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

chore: return lnurlp status and error message when no data can be retrieved #8

wants to merge 3 commits into from

Conversation

rolznz
Copy link
Contributor

@rolznz rolznz commented Jun 9, 2023 •
edited
Loading

Fixes #7

Currently returns a response like:

{
    "lnurlp": null,
    "keysend": null,
    "nostr": null,
    "error": {
        "status": 404,
        "message": "No details: https://getalby.com/.well-known/lnurlp/somenonexistentusername - <nil>"
    }
}

I am wondering if it would be better to return a response with errors for each:

errors: {
  lnurlp: {status: ..., message: ...},
  keysend: {status: ..., message: ...},
  nostr: {status: ..., message: ...}
}

What do you guys think?

@rolznz rolznz requested review from bumi and im-adithya June 9, 2023 09:39
@bumi
Copy link
Contributor

bumi commented Jun 13, 2023

yes, I think error responses for each entry (lnurlp/keysend/nostr) would be good and more consistent there.

@rolznz
Copy link
Contributor Author

rolznz commented Jun 29, 2023

@bumi @im-adithya I have updated the PR. Please re-review when you get the chance

@rolznz
Copy link
Contributor Author

rolznz commented Jun 29, 2023

Example output:

{
    "lnurlp": {
        "allowsNostr": true,
        "callback": "https://getalby.com/lnurlp/test/callback",
        "commentAllowed": 255,
        "maxSendable": 11000000000,
        "metadata": "[[\"text/identifier\",\"[email protected]\"],[\"text/plain\",\"Sats for test\"]]",
        "minSendable": 1000,
        "nostrPubkey": "79f00d3f5a19ec806189fcab03c1be4ff81d18ee4f653c88fac41fe03570f432",
        "payerData": {
            "email": {
                "mandatory": false
            },
            "name": {
                "mandatory": false
            },
            "pubkey": {
                "mandatory": false
            }
        },
        "status": "OK",
        "tag": "payRequest"
    },
    "keysend": {
        "customData": [
            {
                "customKey": "696969",
                "customValue": "ZcZq54ODsfKsqaBi74vs"
            }
        ],
        "pubkey": "030a58b8653d32b99200a2334cfe913e51dc7d155aa0116c176657a4f1722677a3",
        "status": "OK",
        "tag": "keysend"
    },
    "nostr": null,
    "errors": {
        "lnurlp": null,
        "keysend": null,
        "nostr": {
            "status": 404,
            "message": "No details: https://getalby.com/.well-known/nostr.json?name=test - <nil>"
        }
    }
}

@bumi
Copy link
Contributor

bumi commented Jul 24, 2023

oha, this got missed.

@rolznz @im-adithya are our tools compatible with this? does this break something fro somebody?

@bumi
Copy link
Contributor

bumi commented Jul 24, 2023

actually shouldn't the errors be nested in lnurlp etc.?

@rolznz
Copy link
Contributor Author

rolznz commented Jul 25, 2023 •
edited
Loading

@bumi what's the benefit of putting the errors inside? now you have to also make sure lnurlp.error is null before using it for example. (This is probably also less likely to be backward compatible than the current solution)

@rolznz
Copy link
Contributor Author

rolznz commented Jul 25, 2023

oha, this got missed.

@rolznz @im-adithya are our tools compatible with this? does this break something fro somebody?

This should be backward compatible currently, but it will not if we put the errors inside the existing objects (for example alby-tools will need some changes, not sure about any other consumers of this service)

@bumi
Copy link
Contributor

bumi commented Jul 25, 2023

We can keep it both for a while.

but errors within the objects (lnurl, etc.) make most sense, doesn't it?

@bumi
Copy link
Contributor

bumi commented Aug 22, 2023

@im-adithya @rolznz what do we do here?

@rolznz
Copy link
Contributor Author

rolznz commented Aug 22, 2023

@bumi I think you are right that errors make sense to be inside the objects, however this will break all current versions of alby-tools.

So I think we should update alby-tools first to make sure the new format will be handled correctly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bumi bumi Awaiting requested review from bumi

@im-adithya im-adithya Awaiting requested review from im-adithya

Assignees

@im-adithya im-adithya

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Include errors in response
3 participants
@rolznz @bumi @im-adithya