Skip to content

Allow 0 pct forwards to pass only in the client #2281

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

Closed
wants to merge 1 commit into from
Closed

Allow 0 pct forwards to pass only in the client #2281

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion components/adv-post-form.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ export default function AdvPostForm ({ children, item, sub, storageKeyPrefix })
name={`forward[${index}].pct`}
type='number'
step={5}
min={1}
min={0}
max={100}
append={<InputGroup.Text className='text-monospace'>%</InputGroup.Text>}
groupClassName={`${styles.percent} mb-0`}
Expand Down
4 changes: 2 additions & 2 deletions lib/form.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,15 @@ import { hasDeleteMention, hasReminderMention } from './item'

/**
* Normalize an array of forwards by converting the pct from a string to a number
* Also extracts nym from nested user object, if necessary
* Also extracts nym from nested user object, if necessary and removes 0 pct fowards
* @param {*} forward Array of forward objects ({nym?: string, pct: string, user?: { name: string } })
* @returns normalized array, or undefined if not provided
*/
export const normalizeForwards = (forward) => {
if (!Array.isArray(forward)) {
return undefined
}
return forward.filter(fwd => fwd.nym || fwd.user?.name).map(fwd => ({ nym: fwd.nym ?? fwd.user?.name, pct: Number(fwd.pct) }))
return forward.filter(fwd => (fwd.nym || fwd.user?.name) && Number(fwd.pct) > 0).map(fwd => ({ nym: fwd.nym ?? fwd.user?.name, pct: Number(fwd.pct) }))
}

export const toastUpsertSuccessMessages = (toaster, upsertResponseData, dataKey, itemText) => {
Expand Down
2 changes: 1 addition & 1 deletion lib/validate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ export function advPostSchemaMembers ({ me, existingBoost = 0, ...args }) {
},
message: 'cannot forward to yourself'
}),
pct: intValidator.required('must specify a percentage').min(1, 'percentage must be at least 1').max(100, 'percentage must not exceed 100')
pct: intValidator.required('must specify a percentage').min(0, 'percentage must be at least 0').max(100, 'percentage must not exceed 100')
Copy link
Member

@ekzyis ekzyis Jul 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Mhh, this will allow to insert 0 pct forwards via the API because the server doesn't run normalizeForwards. Tested using the GraphQL playground at /api/graphql:

mutation upsertDiscussionWithForwards(
  $title: String!,
  $text: String!,
  $sub: String!,
  $forward: [ItemForwardInput]
) {
  upsertDiscussion(
    title: $title,
    text: $text,
    sub: $sub,
    forward: $forward
  ) {
    result {
      id
    }
  }
}

variables:

{
  "title": "foobar",
  "text": "asdf",
  "sub": "art",
  "forward": [{"nym":"anon","pct":0}]
}

I wish we could just do this:

diff --git a/lib/validate.js b/lib/validate.js
index 576687e6..68b5b204 100644
--- a/lib/validate.js
+++ b/lib/validate.js
@@ -129,6 +129,7 @@ export function advPostSchemaMembers ({ me, existingBoost = 0, ...args }) {
       }),
     forward: array()
       .max(MAX_FORWARDS, `you can only configure ${MAX_FORWARDS} forward recipients`)
+      .transform(forwards => forwards.filter(fwd => Number(fwd.pct) > 0))
       .of(object().shape({
         nym: string().required('must specify a stacker')
           .test({

but we can't since the transform only applies to the array during validation, the submitted array will be unmodified.

Maybe the proper fix would be to make validation return the transformed values in general; or only do so on the client, but the server always validates what it receives without any transforms. But this is a more general thing we would have to change in our code base.

Not sure how to fix this problem of allowing the client to submit something that the server shouldn't accept without too many changes and not making it confusing how it works :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback @ekzyis , I'll work to resolve this.

Curiously, do you use real sats in dev? I'd like to test this more before submitting the next change.

Copy link
Member

@ekzyis ekzyis Jul 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Curiously, do you use real sats in dev? I'd like to test this more before submitting the next change.

No, we don't, we have a regtest lightning network with multiple nodes.

When you set COMPOSE_PROFILES=minimal,payments in .env.local before you run sndev start, it will start them. See this section in the README.

But you can also use CCs via sndev set_balance <user> <balance>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the clarification

Copy link
Contributor Author

@hkarani hkarani Jul 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Mhh, this will allow to insert 0 pct forwards via the API because the server doesn't run normalizeForwards. Tested using the GraphQL playground at /api/graphql

I've tested this:
When normalizeForwards doesn't filter 0 pct forwards

Screen.Recording.2025-07-29.at.21.18.53.mov

Here we actually filter 0 before calling the mutation

https://github.com/stackernews/stacker.news/pull/2281/files#r2240699282

In use-item-submit

stacker.news/components/use-item-submit.js

Line 19 in 6aeffa7

export default function useItemSubmit (mutation,
, upsertItem gets the the forwards after they are 0 pct items are filtered

stacker.news/components/use-item-submit.js

Line 62 in 6aeffa7

forward: normalizeForwards(values.forward),

I still consider this the cleanest solution with no many code changes.

You can try it, test it yourself from the client and check the db for the items Forwards pcts, if this doesn't work for all intents and puporses this PR should be closed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

O-pct.forwards.filtered.mov

Copy link
Member

@ekzyis ekzyis Aug 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

You were looking at code that only runs in the client. What I meant here was that in theory, one does not have to use the client to make requests to our API. I gave you the GraphQL playground as an example.

As-is, the server does not filter out zero pct forwards but assumes the client already did it.

It is important in general that the server does not trust the client with inputs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Caught that

}))
.compact((v) => !v.nym && !v.pct)
.test({
Expand Down