-
-
Notifications
You must be signed in to change notification settings - Fork 131
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.
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
base: master
Are you sure you want to change the base?
Conversation
7f7e13e
to
0bc3d89
Compare
@@ -145,7 +145,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') |
There was a problem hiding this comment.
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 :/
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification
Description
A clear and concise description of what you changed and why.
The client validation is changed to allow 0 by:
To only allow 0 to only pass the client:
Screenshots
Additional Context
Was anything unclear during your work on this PR? Anything we should definitely take a closer look at?
No
Checklist
Are your changes backward compatible? Please answer below:
For example, a change is not backward compatible if you removed a GraphQL field or dropped a database column.
Only form validation and is changed when forwards are normalized.
On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:
8
For frontend changes: Tested on mobile, light and dark mode? Please answer below:
Yes
Did you introduce any new environment variables? If so, call them out explicitly here:
No