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

Allow zapping, posting and commenting without funds or an account #336

Merged
merged 59 commits into from
Aug 11, 2023

Conversation

ekzyis
Copy link
Member

@ekzyis ekzyis commented Jun 28, 2023

Closes #266


Current state (444ebf7): (outdated)

2023-06-28.05-27-07.mp4

TODO:

  • Fix missing apollo cache update after an invoice was paid (outdated)
  • test zapping when logged in still works as before
  • stop polling invoice after confirmation since it will be deleted shortly after (using skip seemed to throw a bigger error and the current "invoice not found" error is only logged to console so I think stopping the query is not needed 🤔) (mhh, the user may see "<div>error</div>" rendered for a very short amount of time before the modal is closed and the mutation is executed... this could confuse users so should be prevented)
  • "allow stackers to zap arbitrary amounts even if their wallet doesn't have enough sats ... by presenting an invoice for them to pay in a modal."
  • replace invoice deletion with a different method to make sure invoices are not double-spent
  • exclude anon user from trust algorithm
  • error handling: let user repeat mutation or contact admins if invoice was paid but error happened during mutation
  • itemRepetition costs are not properly updated

@huumn, this is ready for review now (82d94b2)

here are some showcase videos: https://files.ekzyis.com/public/sn/266-zaps-without-account/

001.mp4 shows the changes for anon users

002.mp4 shows the error handling

003.mp4 shows that stackers can now pay invoices if they don't have enough funds


TODO:

@ekzyis ekzyis marked this pull request as draft June 28, 2023 03:18
components/item-act.js Outdated Show resolved Hide resolved
@ekzyis ekzyis added the feature new product features that weren't there before label Jun 28, 2023
api/resolvers/wallet.js Outdated Show resolved Hide resolved
api/resolvers/wallet.js Outdated Show resolved Hide resolved
@huumn
Copy link
Member

huumn commented Jul 3, 2023

An alternative to rewriting a lot of the code/data models to be user-less is creating an @anon user (or similar) and all anon users perform all their actions through that user.

The minimal code rewrite flow for anon zapping might be: pay invoice to @anon's account, have @anon zap the item the amount of the invoice. (Doing this atomically might be tricky though.)

Implementing anon posting/commenting will likely be a lot easier if user-less actions are done this way afaict. We'd also get fee escalation for the @anon posters/commenters that operates on the group.

@ekzyis ekzyis force-pushed the 266-zaps-without-account branch 5 times, most recently from 1809a02 to 87ec14a Compare July 11, 2023 22:22
@ekzyis
Copy link
Member Author

ekzyis commented Jul 11, 2023

An alternative to rewriting a lot of the code/data models to be user-less is creating an @anon user (or similar) and all anon users perform all their actions through that

Done 👍

Doing this atomically might be tricky though.

I did this by adding a column itemId to the invoice table and updating the confirm_invoice function. If an invoice has an itemId set, confirm_invoice will now trigger a zap by calling item_act with that item id and the invoice user id.

I also had to update the serialize function though since I wasn't able to create an invoice and set the item id in one query. I noticed that CTEs all run with the same snapshot, so the following didn't work:

WITH created_invoice AS (
  SELECT id FROM create_invoice(...)
)
UPDATE "Invoice"
SET "itemId" = $1
FROM created_invoice
WHERE "Invoice".id = created_invoice.id

The sub-statements in WITH are executed concurrently with each other and with the main query. Therefore, when using data-modifying statements in WITH, the order in which the specified updates actually happen is unpredictable. All the statements are executed with the same snapshot (see Chapter 13), so they cannot “see” one another's effects on the target tables.

-- https://www.postgresql.org/docs/current/queries-with.html

New state with proxy anon user instead of deep invoice integration (87ec14a):

2023-07-12.00-15-49.mp4

api/resolvers/item.js Outdated Show resolved Hide resolved
lib/constants.js Outdated Show resolved Hide resolved
@huumn
Copy link
Member

huumn commented Jul 11, 2023

It looks pretty good on first glance. We have quite a few options for making it generic that we might want to consider before merging (so that we can support a wide variety of anonymous actions).

Options for storing on backend and waiting for payment

  1. polymophic has-one relationship in "Invoice"
    • create anon versions of all tables that anons might act on by inheriting from the non-anon table
      • e.g. "Item" -> "PeningItem", "ItemAct" -> "PendingItemAct"
    • then add "Invoice"."pendingActionId" and "Invoice"."pendingActionType"
    • when the invoice is confirmed insert into non-anon tables, e.g. insert into "Item" select * from "PendingItem" where "Item".id = "Invoice"."pendingActionId"
  2. Store the "actions" as JSON in the "Invoice"
    • we'd then create the actions when the invoice is confirmed
  3. Add a "invoiceHash" field to "Item" and "ItemAct" and don't display the "Item" or "ItemAct" until confirmed

Options for only sending to backend when payment confirmed

There's only really one way to do this (which might indicate it's the right way):

  • poll backend for payment confirmation
  • when confirmed, send mutation with the invoice hash
  • backend confirms invoice hash is paid, indicates somehow that the invoice hash has been spent, and stores the mutation

IMO the server-side versions seem very complicated if we're going to allow anons to really use SN. The client-side version isn't atomic but we can cache the invoice hashes so that they can be retried on the rare failure cases (and possibly even refunded in extreme cases).

api/resolvers/item.js Outdated Show resolved Hide resolved
@huumn
Copy link
Member

huumn commented Jul 11, 2023

Another "dimension" of generic that we might want to consider is allowing logged in users to keep a zero/low balance and pay/zap only when they perform actions.

Most of the generic options above are consistent with that though I think.

@ekzyis
Copy link
Member Author

ekzyis commented Jul 12, 2023

Options for only sending to backend when payment confirmed

There's only really one way to do this (which might indicate it's the right way):

  • poll backend for payment confirmation
  • when confirmed, send mutation with the invoice hash
  • backend confirms invoice hash is paid, indicates somehow that the invoice hash has been spent, and stores the mutation

IMO the server-side versions seem very complicated if we're going to allow anons to really use SN. The client-side version isn't atomic but we can make cache the invoice hashes so that they can be retried on the rare failure cases

That does indeed sound good. I'll update the code to work like this

@huumn
Copy link
Member

huumn commented Jul 12, 2023

The only gotcha I can see with this approach is post/comment fee escalation race conditions, e.g.

I pay when there isn't another recent anon post, but before I submit my mutation another anon submits theirs, and then my mutation fails because I paid 1 * 10^0 sats.

But as we discussed elsewhere, perhaps we just make anon posts/comments 1000 sats or something and disable fee escalation.

@ekzyis
Copy link
Member Author

ekzyis commented Jul 13, 2023

a9ffee3 now uses attached invoice ids as a proof of payment when sending a mutation.

To prevent double spends, the invoice is deleted inside the same transaction.

It's missing code for the following though:

The client-side version isn't atomic but we can cache the invoice hashes so that they can be retried on the rare failure cases (and possibly even refunded in extreme cases).

@ekzyis
Copy link
Member Author

ekzyis commented Jul 19, 2023

Added commenting and posting as anons (links, discussions and polls) now and disabled fee escalation for the anon user. I have set anon comment fees to 100 and post fees to 1000.

TODO:

  • stop polling invoice after confirmation since it will be deleted shortly after
  • "allow stackers to zap arbitrary amounts even if their wallet doesn't have enough sats ... by presenting an invoice for them to pay in a modal."

no showcase video since Github limits uploads to 10MB and my video was too long lol. I'll may upload them to my own server and put a link here

edit: added these TODOs to the issue description

@ekzyis
Copy link
Member Author

ekzyis commented Jul 20, 2023

I realized that double spends are still possible since you can use existing payment hashes (which were used for depositing) to pay for stuff since the backend only checks if there was a paid invoice with that payment hash to allow actions.

So we need to use a different method than deletion to check if an invoice was already spent

edit: Okay, just tested it using a deposit payment hash with curl:

$ curl -XPOST -H "Content-type: application/json" --data '{"query": "mutation act { act(invoiceHash: \"7d778c35544f98867c259f6ee821f182873a0065b3458d5fa7f88a4a500fe95c\", id: 184082, sats: 100000) { sats } }"}' https://sn.ekzyis.com/api/graphql
{"errors":[{"message":"insufficient funds","locations":[{"line":1,"column":16}],"path":["act"],"extensions":{"code":"BAD_USER_INPUT","exception":{"stacktrace":["UserInputError: insufficient funds","    at retry.m
inTimeout (webpack-internal:///(api)/./api/resolvers/serial.js:16:14)","    at runMicrotasks (<anonymous>)","    at processTicksAndRejections (node:internal/process/task_queues:96:5)","    at async serialize (web
pack-internal:///(api)/./api/resolvers/serial.js:8:10)","    at async Object.act (webpack-internal:///(api)/./api/resolvers/item.js:1439:12)"]}}}],"data":null}

So I forgot that the postgres function item_act still deducts the balance from the user and no double-spent is possible like this.

So we can continue to use invoice deletion.

For example, the following works but 50k sats are correctly deducted from my balance while doing so:

$ curl -v -XPOST -H "Content-type: application/json" --data '{"query": "mutation act { act(invoiceHash: \"7d778c35544f98867c259f6ee821f182873a0065b3458d5fa7f88a4a500fe95
c\", id: 184082, sats: 50000) { sats } }"}' https://sn.ekzyis.com/api/graphql
{"data":{"act":{"sats":50000}}}

@huumn
Copy link
Member

huumn commented Jul 20, 2023

note: I haven't looked at the code

If a user is logged in, we could prevent them from using payment hashes. Instead, their paid invoice always goes to their balance, and they always spend from their balance.

@ekzyis ekzyis changed the title Support zaps without account Allow zapping, posting and commenting without funds or an account Jul 21, 2023
@ekzyis
Copy link
Member Author

ekzyis commented Jul 21, 2023

I thought I am done now but I think there is one thing missing:
@huumn, iirc, I think we discussed that the anon user should never gain any trust from a) trusted people zapping it or b) it zapping posts other people also like (is that a good summary how trust is gained? i'm still a noob regarding this) since I think the anon user should not be able to affect ranking, right?

I am going to create a showcase video with all features soon and upload it to my own server. Creating these videos also helps me in making sure everything works as expected

@huumn
Copy link
Member

huumn commented Jul 21, 2023

You'd just skip adding them to values in worker/trust.js:160

@huumn huumn merged commit b9461b7 into stackernews:master Aug 11, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature new product features that weren't there before
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow zapping without funds or an account
2 participants