Funding Request Proposal for Multiple Recipients #2365 #4453

vrrayz · 2023-06-21T04:14:02Z

Ref to this issue #2365

Create Proposal Scope Completed

Added "Pay multiple" toggle which when triggered addds a text are input
Added prompt on the format of adding accounts
Added Textarea for Destination accounts and transfer amounts
Added CSV Format Validation for the TextArea
Added Maximum accounts Validation
Added Max payment validation
Added Address validation
Added Preview Modal with Validation

vercel · 2023-06-21T04:14:06Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
dao	✅ Ready (Inspect)	Visit Preview	Aug 11, 2023 2:41pm
pioneer-2	✅ Ready (Inspect)	Visit Preview	Aug 11, 2023 2:41pm
pioneer-2-storybook	✅ Ready (Inspect)	Visit Preview	Aug 11, 2023 2:41pm

traumschule

This looks great and works perfectly up to proposal creation: For some reason tx fails with

proposalsCodex.InvalidFundingRequestProposalBalance

Is the total amount submitted correctly?

Editor

100 for alice and bob each

Validation

Preview

vrrayz · 2023-06-21T08:10:32Z

@traumschule I forgot to add in the new parameters that support multiple funding requests. My bad!.
I just added a new commit now to fix that here

traumschule

Proposal 374 #2788397

thesan

@vrrayz thank you for this PR and sorry it took me so long to finally get to review it. You did a really nice work with this proposal that's really not an easy one !

However there is a bunch of changes I believe this feature needs. In addition to those change requests there is the main reason it took me so long to get to this PR: which is that this proposal should get at least one automated test. Please add a new "Specific Parameters Multiple Recipients Funding Request" test story right after this one: https://pioneer-2-storybook-joystream.vercel.app/?path=/story/pages-proposals-proposallist-current--specific-parameters-funding-request

thesan · 2023-07-18T15:08:25Z

packages/ui/src/proposals/modals/AddNewProposal/helpers.ts

+ .test(duplicateAccounts('Duplicate accounts are not allowed'))
+ .test(isValidCSV('Not valid CSV format'))
+ .test(
+ maxFundingAmount(
+ 'Maximal amount allowed is ${max}',
+ api?.consts.proposalsCodex.fundingRequestProposalMaxTotalAmount
+ )
+ )
+ .test(
+ maxAccounts(
+ 'Maximum allowed accounts is ${max}',
+ api?.consts.proposalsCodex.fundingRequestProposalMaxAccounts.toNumber()
+ )
+ )


If I understand the design correctly, all these validations except for isValidCSV should be shown in the PreviewAndValidateModal for clarity. So the form validation should only check whether the csv format is correct.

Suggested change

.test(duplicateAccounts('Duplicate accounts are not allowed'))

.test(isValidCSV('Not valid CSV format'))

.test(

maxFundingAmount(

'Maximal amount allowed is ${max}',

api?.consts.proposalsCodex.fundingRequestProposalMaxTotalAmount

)

)

.test(

maxAccounts(

'Maximum allowed accounts is ${max}',

api?.consts.proposalsCodex.fundingRequestProposalMaxAccounts.toNumber()

)

)

.test(isValidCSV('Not valid CSV format'))

packages/ui/src/proposals/modals/AddNewProposal/helpers.ts

thesan · 2023-07-18T15:36:55Z

packages/ui/src/proposals/modals/AddNewProposal/helpers.ts

+ is: false,
+ then: (schema) =>
+ schema
+ .test(moreThanMixed(0, '')) // todo: change funding request to allow upload request in file


This comment can be removed now:

Suggested change

.test(moreThanMixed(0, '')) // todo: change funding request to allow upload request in file

.test(moreThanMixed(0, ''))

thesan · 2023-07-18T15:57:42Z

packages/ui/src/proposals/modals/AddNewProposal/getSpecificParameters.ts

+ const decimals = new BN(10).pow(new BN(JOY_DECIMAL_PLACES))
+ return input?.split(';\n').map((item) => {
+ const [address, amount] = item.split(',')
+ const formattedAmount = new BN(amount).mul(decimals)


This doesn't work when the amount has a decimal part because bn.js doesn't work with decimals see:

> new BN('1.5').muln(2).toNumber() 2

There is already the joy helper function for this:

Suggested change

const formattedAmount = new BN(amount).mul(decimals)

const formattedAmount = new BN(joy(amount))

thesan · 2023-07-18T16:08:46Z

...Proposal/components/SpecificParameters/modals/PreviewAndValidate/PreviewAndValidateModal.tsx

+ setErrorMessages((prev) =>
+ totalInvalidAccounts > 0 ? [...prev, 'Incorrect destination accounts detected'] : [...prev]
+ )
+ setErrorMessages((prev) =>
+ maxAllowedAccounts && previewAccounts?.length > maxAllowedAccounts
+ ? [...prev, 'Maximum allowed accounts exceeded']
+ : [...prev]
+ )


Please avoid calling set state dispatchers when no data change is needed because it creates useless re-renders. Instead just call them conditionally:

Suggested change

setErrorMessages((prev) =>

totalInvalidAccounts > 0 ? [...prev, 'Incorrect destination accounts detected'] : [...prev]

)

setErrorMessages((prev) =>

maxAllowedAccounts && previewAccounts?.length > maxAllowedAccounts

? [...prev, 'Maximum allowed accounts exceeded']

: [...prev]

)

if (totalInvalidAccounts > 0) {

setErrorMessages((prev) => [...prev, 'Incorrect destination accounts detected'])

}

if (maxAllowedAccounts && previewAccounts?.length > maxAllowedAccounts) {

setErrorMessages((prev) => [...prev, 'Maximum allowed accounts exceeded'])

}

Also are you sure calling these 2 setErrorMessages in the same useEffect works ? meaning the prev value gets updated between the 2 calls ? It probably does but I have a small doubt. Just in case you haven't tested that part already.

thesan · 2023-07-18T16:15:17Z

...Proposal/components/SpecificParameters/modals/PreviewAndValidate/PreviewAndValidateModal.tsx

+ setIsPreviewModalShown,
+ previewModalData,
+ setValue,


You shouldn't need to pass these as props. I think you can access these with:

const { setValue, getValues } = useFormContext()

just like in the FundingRequest form.

thesan · 2023-07-18T16:40:25Z

...ages/ui/src/proposals/modals/AddNewProposal/components/SpecificParameters/FundingRequest.tsx

+ const verifyInput = useCallback((input: string) => {
+ setValue('fundingRequest.hasPreviewedInput', false, { shouldValidate: true })
+ setValue('fundingRequest.accountsAndAmounts', input, { shouldValidate: true })
+ }, [])


This looks like a dangerous hack to me. Instead please watch the csv value (currently accountsAndAmounts) and reset what should be reset (currently hasPreviewedInput) in a useEffect.

So with the current accountsAndAmounts and hasPreviewedInput it would be something like:

const accountsAndAmounts = watch('accountsAndAmounts') useEffect(() => { if (getFieldState('hasPreviewedInput')) { setData('fundingRequest.hasPreviewedInput', false, { shouldValidate: true }) } }, [accountsAndAmounts])

packages/ui/src/common/components/Modal/Modal.tsx

thesan

Thank you, the tests are perfect !

There is just one last thing to change in term of UX:

When the csv field is empty or invalid please:

hide the "Please preview and validate the inputs to proceed" message
disable the "Preview and Validate" button

Also I just realized that a followup to this PR will be needed because ATM the preview page only shows 1 of the payments. I setup a temporary PR to visualize this case in storybook: https://pioneer-2-storybook-git-fork-thesan-feature-mu-ca321d-joystream.vercel.app/?path=/story/pages-proposals-proposalpreview--funding-request-multiple-recipients

So I'll retarget this PR to a new branch.

…ssue-2365

thesan · 2023-08-11T11:56:31Z

packages/ui/src/common/components/Modal/Modal.tsx

+ customModalSize?: any
+ marginRight?: any


I just realized that the changes to this file are now unnecessary. Please run: git checkout dev -- packages/ui/src/common/components/Modal/Modal.tsx to reverse these changes.

thesan · 2023-08-11T12:03:18Z

packages/ui/src/proposals/modals/AddNewProposal/AddNewProposalModal.tsx

@@ -24,6 +24,7 @@ import { TextMedium, TokenValue } from '@/common/components/typography'
 import { BN_ZERO } from '@/common/constants'
 import { camelCaseToText } from '@/common/helpers'
 import { useCurrentBlockNumber } from '@/common/hooks/useCurrentBlockNumber'
+import { useKeyring } from '@/common/hooks/useKeyring'


Same thing for this file since the validation doesn't happens in the useYupValidationResolver anymore

thesan · 2023-08-11T12:04:53Z

packages/ui/src/proposals/model/validation.ts

+export const maxAccounts = (message: string, max: number | undefined): Yup.TestConfig<any, AnyObject> => ({
+ message,
+ name: 'maxAccounts',
+ params: { max },
+ exclusive: false,
+ test(value: string) {
+ const pairs = value.split('\n')
+ return max ? pairs.length <= max : false
+ },
+})
+
+export const duplicateAccounts = (message: string): Yup.TestConfig<any, AnyObject> => ({
+ message,
+ name: 'duplicateAccounts',
+ exclusive: false,
+ test(value: string) {
+ const pairs = value.split('\n')
+ const addresses: string[] = []
+
+ for (const pair of pairs) {
+ const [address] = pair.split(',')
+ if (addresses.indexOf(address) >= 0) return false
+ addresses.push(address)
+ }
+
+ return true
+ },
+})
+
+export const isValidCSV = (message: string): Yup.TestConfig<any, AnyObject> => ({
+ message,
+ name: 'isValidCSV',
+ exclusive: false,
+ test(value: string) {
+ if (!CSV_PATTERN.test(value)) return false
+
+ const pairs = value.split('\n')
+
+ for (const pair of pairs) {
+ const [, amount] = pair.split(',')
+ if (!Number(amount)) return false
+ }
+
+ return true
+ },
+})
+
+export const maxFundingAmount = (
+ message: string,
+ max: Reference<number | BN> | number | BN | undefined,
+ isJoyValue = true
+): Yup.TestConfig<any, AnyObject> => ({
+ message,
+ name: 'maxFundingAmount',
+ params: { max: isJoyValue && isBn(max) ? formatJoyValue(max, { precision: 2 }) : max },
+ exclusive: false,
+ test(value: string) {
+ const pairs = value.split('\n')
+ let total = new BN(0)
+ const decimals = new BN(10).pow(new BN(JOY_DECIMAL_PLACES))
+ for (const pair of pairs) {
+ const [, amount] = pair.split(',')
+ total = total.add(new BN(amount))
+ }
+ total = total.mul(decimals)
+ return !total || typeof max === 'undefined' || !isBn(total) || total.lte(new BN(this.resolve(max)))
+ },
+})


Except for isValidCSV are any of these functions still in use ? If not please remove them

thesan · 2023-08-11T12:16:19Z

...ages/ui/src/proposals/modals/AddNewProposal/components/SpecificParameters/FundingRequest.tsx

+ const shouldDisablePreviewButton = () => {
+ const input = getFieldState('fundingRequest.csvInput')
+ return input.error !== undefined || !input.isDirty
+ }


Since this function is called on every render, it would be simpler to just have a variable:

Suggested change

const shouldDisablePreviewButton = () => {

const input = getFieldState('fundingRequest.csvInput')

return input.error !== undefined || !input.isDirty

}

const canPreviewInput = csvInput.isDirty && !csvInput.error

thesan · 2023-08-11T12:17:21Z

...ages/ui/src/proposals/modals/AddNewProposal/components/SpecificParameters/FundingRequest.tsx

+ {shouldPreview() && <ErrorPrompt>Please preview and validate the inputs to proceed</ErrorPrompt>}
+ <ButtonPrimary
+ size="medium"
+ disabled={shouldDisablePreviewButton()}


So this becomes:

Suggested change

disabled={shouldDisablePreviewButton()}

disabled={!canPreviewInput}

thesan · 2023-08-11T12:21:09Z

...ages/ui/src/proposals/modals/AddNewProposal/components/SpecificParameters/FundingRequest.tsx

+ />
+ </InputComponent>
+ <HiddenCheckBox name="fundingRequest.hasPreviewedInput" checked={hasPreviewedInput} />
+ {shouldPreview() && <ErrorPrompt>Please preview and validate the inputs to proceed</ErrorPrompt>}


And this:

Suggested change

{shouldPreview() && <ErrorPrompt>Please preview and validate the inputs to proceed</ErrorPrompt>}

{canPreviewInput && !hasPreviewedInput && <ErrorPrompt>Please preview and validate the inputs to proceed</ErrorPrompt>}

I might have missed something but AFAICS this should be enough.

thesan

It's all good. Great work 🙇

vrrayz added 4 commits June 20, 2023 22:28

preview multiple receipients modal

254936f

multiple funding request validation

dead262

csv reg exp pattern

4d0bfa0

Preview And Validate Modal validation

30651dc

vercel bot deployed to Preview – pioneer-2-storybook June 21, 2023 04:16 View deployment

vercel bot deployed to Preview – pioneer-2 June 21, 2023 04:18 View deployment

vercel bot deployed to Preview – dao June 21, 2023 04:18 View deployment

lint fix

e7517cc

vercel bot deployed to Preview – pioneer-2-storybook June 21, 2023 04:25 View deployment

vercel bot deployed to Preview – pioneer-2 June 21, 2023 04:27 View deployment

vercel bot deployed to Preview – dao June 21, 2023 04:27 View deployment

Tx fail fix

e7d4b82

traumschule suggested changes Jun 21, 2023

View reviewed changes

traumschule added community-dev issue suitable for community-dev pipeline qa-tests-failed builders-wg-code-review labels Jun 21, 2023

vercel bot deployed to Preview – dao June 21, 2023 08:10 View deployment

vercel bot deployed to Preview – pioneer-2-storybook June 21, 2023 08:11 View deployment

vercel bot deployed to Preview – pioneer-2 June 21, 2023 08:12 View deployment

thesan mentioned this pull request Jun 21, 2023

🔧 Fix the storybook CI #4452

Merged

traumschule approved these changes Jun 21, 2023

View reviewed changes

traumschule added jsg-code-review and removed qa-tests-failed builders-wg-code-review labels Jun 21, 2023

traumschule closed this Jun 21, 2023

traumschule reopened this Jun 21, 2023

thesan requested changes Jul 18, 2023

View reviewed changes

thesan removed the jsg-code-review label Jul 21, 2023

vercel bot deployed to Preview – pioneer-2 August 7, 2023 12:36 View deployment

lint

172518f

vercel bot deployed to Preview – pioneer-2-storybook August 7, 2023 12:39 View deployment

vercel bot deployed to Preview – pioneer-2 August 7, 2023 12:41 View deployment

vercel bot deployed to Preview – dao August 7, 2023 12:42 View deployment

thesan requested changes Aug 9, 2023

View reviewed changes

thesan changed the base branch from dev to feature/multi-recipient-proposal August 9, 2023 16:09

vrrayz and others added 3 commits August 11, 2023 10:19

disable preview button

617ab12

Merge branch 'dev' of https://github.com/vrrayz/pioneer into issue-2365

18b3cf5

Merge branch 'Joystream:dev' into issue-2365

904d370

vercel bot deployed to Preview – pioneer-2-storybook August 11, 2023 09:35 View deployment

vercel bot deployed to Preview – pioneer-2 August 11, 2023 09:35 View deployment

vercel bot deployed to Preview – dao August 11, 2023 09:36 View deployment

vrrayz added 2 commits August 11, 2023 11:15

updated storybook test for multiple funding request

59f10f2

Merge branch 'issue-2365' of https://github.com/vrrayz/pioneer into i…

9a3d8bb

…ssue-2365

vercel bot deployed to Preview – pioneer-2-storybook August 11, 2023 10:23 View deployment

vercel bot deployed to Preview – pioneer-2 August 11, 2023 10:25 View deployment

vercel bot deployed to Preview – dao August 11, 2023 10:25 View deployment

thesan requested changes Aug 11, 2023

View reviewed changes

vrrayz added 2 commits August 11, 2023 13:46

requested changes

313eb98

requested changes

f58a6fb

vercel bot deployed to Preview – pioneer-2-storybook August 11, 2023 14:40 View deployment

vercel bot deployed to Preview – dao August 11, 2023 14:41 View deployment

vercel bot deployed to Preview – pioneer-2 August 11, 2023 14:41 View deployment

thesan approved these changes Aug 11, 2023

View reviewed changes

thesan merged commit 83b4ef3 into Joystream:feature/multi-recipient-proposal Aug 16, 2023
3 checks passed

vrrayz mentioned this pull request Aug 30, 2023

Funding request multiple recipient preview #4509

Merged

chrlschwb added qa-task and removed jsg-code-review labels Sep 15, 2023

chrlschwb removed the qa-task label Sep 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Funding Request Proposal for Multiple Recipients #2365 #4453

Funding Request Proposal for Multiple Recipients #2365 #4453

vrrayz commented Jun 21, 2023

vercel bot commented Jun 21, 2023 •

edited

Loading

traumschule left a comment

vrrayz commented Jun 21, 2023

traumschule left a comment

thesan left a comment

thesan Jul 18, 2023

thesan Jul 18, 2023

thesan Jul 18, 2023

thesan Jul 18, 2023

thesan Jul 18, 2023

thesan Jul 18, 2023

thesan left a comment

thesan Aug 11, 2023

thesan Aug 11, 2023

thesan Aug 11, 2023

thesan Aug 11, 2023

thesan Aug 11, 2023

thesan Aug 11, 2023

thesan left a comment

	.test(moreThanMixed(0, '')) // todo: change funding request to allow upload request in file
	.test(moreThanMixed(0, ''))

	const formattedAmount = new BN(amount).mul(decimals)
	const formattedAmount = new BN(joy(amount))

	disabled={shouldDisablePreviewButton()}
	disabled={!canPreviewInput}

	{shouldPreview() && <ErrorPrompt>Please preview and validate the inputs to proceed</ErrorPrompt>}
	{canPreviewInput && !hasPreviewedInput && <ErrorPrompt>Please preview and validate the inputs to proceed</ErrorPrompt>}

Funding Request Proposal for Multiple Recipients #2365 #4453

Funding Request Proposal for Multiple Recipients #2365 #4453

Conversation

vrrayz commented Jun 21, 2023

Create Proposal Scope Completed

vercel bot commented Jun 21, 2023 • edited Loading

traumschule left a comment

Choose a reason for hiding this comment

Editor

Validation

Preview

vrrayz commented Jun 21, 2023

traumschule left a comment

Choose a reason for hiding this comment

thesan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thesan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thesan left a comment

Choose a reason for hiding this comment

vercel bot commented Jun 21, 2023 •

edited

Loading