-
Notifications
You must be signed in to change notification settings - Fork 14
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
feat: added reputation method to release #3162
base: master
Are you sure you want to change the base?
Conversation
417daf8
to
1bd4b28
Compare
@CzarekDryl Can you rebase this so I can give it a review? |
890ca7c
to
0324712
Compare
@iamsamgibbs Branch rebased |
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 your PR @CzarekDryl, nice to see this coming together.
The message ID for Reputation in the release modal seems incorrect and causing errors:
After releasing via a motion, the UI says it was the Payment creator decision method:
The rest of the flow looks good, including voting and failed motions, nice work 👍
@jakubcolony Payment creator box is not displaying anymore |
d78ac13
to
b0d4b0a
Compare
72668b1
to
0558341
Compare
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.
@CzarekDryl Thank you for adding this in. I picked up
-
The text is wrong in the info callout, it should be "for releasing the payment."
-
Is there a way to not have the "Failed" show here before the "Passed"? Can we use the skeleton loader?
failed-shows.webm
0558341
to
bfa399e
Compare
before
after |
@CzarekDryl does this need to be an arbitrary setTimeout then, maybe like 3 seconds? That way, it allows time for the loading without showing a false outcome, but will still fallback to the hopefully rare occasions where a motion that gets to a vote does not receive any votes. |
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 rebase @CzarekDryl, I appreciate this must've not been the easiest one due to the amount of changes.
It looks like there's an unfortunate name clash between the concepts of releasing a payment (finalizing expenditure on the contracts) and releasing a milestone (or stage on the contracts) in case of staged payments. I'd like to suggest we go with the contracts naming as they allow for distinction, would be great if you could rename functions and components to reflect that.
That caused some unexpected UI behaviour:
- Timer shown in two places for staged payments:
- There's a useEffect in
PaymentBuilderWidget
responsible for setting initially selected funding and release (milestone) actions. It's conflicting with the finalizing actions and there isn't one selected by default:
- The action under Payment step matches the action from Release step, which in this case was a motion ready to claim:
const sortedReleaseActions = useMemo( | ||
() => | ||
finalizingActions?.items.filter(notNull).sort((a, b) => { | ||
if (a?.createdAt && b?.createdAt) { | ||
return Date.parse(b.createdAt) - Date.parse(a.createdAt); | ||
} | ||
return 0; | ||
}) ?? [], | ||
[finalizingActions?.items], | ||
); |
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.
You could reuse an existing sortActionsByCreatedDate
util
selectedReleaseAction: ExpenditureAction | null; | ||
setSelectedReleaseAction: (action: ExpenditureAction | null) => void; |
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.
This has to be separate from release milestone action
src/i18n/en.json
Outdated
"releaseModal.permissionsDescription": "This will use your Payer permissions to immediately make the decision and release the payment.", | ||
"releaseModal.reputationDescription": "This will start the lazy consensus process using reputation to make the decision for releasing the payment.", |
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.
The footer in the modal should show the current user's role instead of hardcoded one
@@ -0,0 +1,37 @@ | |||
import React, { type FC } from 'react'; |
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.
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.
I've added new shared component and changed FundingRequests
and FinalizeRequests
but left it in ReleaseActions
because there is some additional logic there
useEffect(() => { | ||
if (!selectedReleaseAction && sortedReleaseActions.length > 0) { | ||
setSelectedReleaseAction(sortedReleaseActions[0]); | ||
} | ||
|
||
if ( | ||
selectedReleaseAction && | ||
!sortedReleaseActions.some( | ||
(releaseAction) => | ||
releaseAction.transactionHash === | ||
selectedReleaseAction.transactionHash, | ||
) | ||
) { | ||
setSelectedReleaseAction(null); | ||
} | ||
}, [selectedReleaseAction, setSelectedReleaseAction, sortedReleaseActions]); |
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.
This useEffect needs to work with a similar one in PaymentBuilderWidget
useEffect(() => { | ||
if ( | ||
!isAnyReleaseMotionInProgress && | ||
allReleaseMotions && | ||
allReleaseMotions.length > 0 && | ||
!allReleaseMotions[0].motionStateHistory.hasPassed | ||
) { | ||
setExpectedStepKey(ExpenditureStep.Release); | ||
} | ||
}, [allReleaseMotions, isAnyReleaseMotionInProgress, setExpectedStepKey]); |
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.
What is this responsible for? Seems like the only thing impacted by this would be the funding button which won't be shown at this stage either way.
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.
It seems to me that I handled some case, but without it, everything also seems to work well
@arrenv the explanation from @CzarekDryl made me think this must be a race condition that also affects regular motions (as it's using the same code). After 3 attempts I managed to reproduce it on a mint tokens motion: Screen.Recording.2024-10-17.at.19.18.06.movAdditionally the voting step shows the wrong outcome: I have opened an issue #3390. |
fix: rebase
3d6c533
to
4d724d4
Compare
Description
Testing
Main issue: #2256