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

cowboy credits (aka nov-5 (aka jan-3)) #1678

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

cowboy credits (aka nov-5 (aka jan-3)) #1678

wants to merge 32 commits into from

Conversation

huumn
Copy link
Member

@huumn huumn commented Dec 3, 2024

This is ready for review. It primarily just gets the important jobs done. Some of more involved things I've discussed like overhauling: settings, top stackers, satistics, etc are prohibitively involved, ie it's too much to change at once a priori like this, ie it's easy to bike shed myself on nonsense things.

TODO

  • receive fee credits for zaps under x sats
    • test
  • send fee credits when zapping under x sats
    • test
  • non-p2p zaps/receives go to fee credits
    • test
    • we don't know if the sender has attached sending wallets, so we need a parameter in the mutation for them to tell us
    • make default behavior for receives: proxy, with a fallback to direct, which falls back to CCs
  • buy fee credits
    • paid action
    • mutation
  • wallet UI needs:
    • buy fee credits
    • withdrawal reward sats
      • test
    • attached wallet pages
  • does satistics still make sense?
    • I've decided it makes enough sense to ship ... after a bunch of half-rewrites, I realized the "right" way will require a massive refactor akin to Remodel DB with paid actions at center? #1717 ... and it's not worth putting that hurdle up to this
    • we'll just change SATS in the final column to SATS/CCs
  • remove p2p zap/invoice notifications
  • zap notifications that list sats/credits
    • note: needs to be backwards compat (ie can't assume all old sat zaps are credits)
  • invites need to use credits
    • should probably be a paid action
    • test
  • item display (need update comments queries to gather these)
    • item.commentSats
    • item.commentCredits
    • item.sats
    • item.credits
    • item.meSats
    • item.meCredits
  • user display
    • me.sats
    • me.credits
    • me.satsStacked punt
    • me.creditsStacked punt
  • squash migrations

@huumn huumn mentioned this pull request Dec 4, 2024
@huumn huumn mentioned this pull request Dec 8, 2024
6 tasks
@huumn huumn marked this pull request as ready for review December 14, 2024 23:38
@huumn huumn changed the title wip cowboy credits (aka nov-5) cowboy credits (aka nov-5 ) Dec 14, 2024
@huumn huumn changed the title cowboy credits (aka nov-5 ) cowboy credits (aka nov-5 (aka jan-3)) Dec 14, 2024
@ekzyis ekzyis self-requested a review December 16, 2024 13:40
Copy link
Member

@ekzyis ekzyis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far! Only looked through all the code and tested buying credits.

Left some questions.

// if the zap is dust, or if me doesn't have a send wallet but has enough sats/credits to pay for it
// then we don't invoice the peer
if (sats < me?.sendCreditsBelowSats ||
(me && !hasSendWallet && (me.mcredits > cost || me.msats > cost))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't the balance checks use >=?

api/paidAction/zap.js Show resolved Hide resolved
api/resolvers/invite.js Show resolved Hide resolved
Comment on lines +203 to +213
if (act === 'TIP' && me) {
return existingSats + sats
}
return existingSats
},
meCredits: (existingCredits = 0) => {
if (act === 'TIP' && !p2p && me) {
return existingCredits + sats
}
return existingCredits
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is me required for the item to stack real sats? an anon zap can also be p2p

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because you made it so that we manage anon sats elsewhere, right?

@@ -535,9 +535,31 @@ function Referral ({ n }) {
)
}

function stackedText (item) {
let text = ''
console.log(item.sats, item.credits)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

console.log 😱

Comment on lines 541 to 553
if (item.sats) {
text += `${numWithUnits(item.sats, { abbreviate: false })}`

if (item.credits) {
text += ' ('
}
}
if (item.credits) {
text += `${numWithUnits(item.credits, { abbreviate: false, unitSingular: 'CC', unitPlural: 'CCs' })}`
if (item.sats) {
text += ')'
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mhh, maybe we should split them so it's also easy to see how many sats were stacked. Currently, the parentheses only give you the exact number of CCs. Also, the item info already shows the same number of sats + credits:

2024-12-18-184909_424x84_scrot

so I am proposing this instead:

2024-12-18-185152_421x77_scrot

naive patch:

diff --git a/components/notifications.js b/components/notifications.js
index faae6f80..98aec7a6 100644
--- a/components/notifications.js
+++ b/components/notifications.js
@@ -537,19 +537,14 @@ function Referral ({ n }) {

 function stackedText (item) {
   let text = ''
-  console.log(item.sats, item.credits)
   if (item.sats) {
-    text += `${numWithUnits(item.sats, { abbreviate: false })}`
-
-    if (item.credits) {
-      text += ' ('
-    }
+    text += `${numWithUnits(item.sats - item.credits, { abbreviate: false })}`
   }
   if (item.credits) {
-    text += `${numWithUnits(item.credits, { abbreviate: false, unitSingular: 'CC', unitPlural: 'CCs' })}`
     if (item.sats) {
-      text += ')'
+      text += ' and'
     }
+    text += ` ${numWithUnits(item.credits, { abbreviate: false, unitSingular: 'CC', unitPlural: 'CCs' })}`
   }

   return text

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right that looks better

@@ -110,6 +110,8 @@ export default function Settings ({ ssrData }) {
// if we switched to anon, me is null before the page is reloaded
if ((!data && !ssrData) || !me) return <PageLoading />

console.log(settings)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

second console.log 😱

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

Successfully merging this pull request may close these issues.

2 participants