Feat/15886093 reputation cancel #3322
Conversation
jakubcolony
force-pushed
the
feat/advanced-payments
branch
from
c0099a8 to
c60c833
Compare
|
@CzarekDryl looks like it needs another rebase on |
CzarekDryl
force-pushed
the
feat/15886093-reputation-cancel
branch
from
87e902e to
f782dda
Compare
|
@jakubcolony @arrenv Branch rebased and ready to review |
CzarekDryl
force-pushed
the
feat/15886093-reputation-cancel
branch
2 times, most recently
from
4113c5c to
255d868
Compare
There was a problem hiding this comment.
@CzarekDryl Thank you for the work on this, and your patience. I have noticed a couple of things.
-
This translation error appeared when the Advanced payment was created.
-
Two cancelled pills appeared, there should be only one pill in the timeline.
-
The staking decision method does not trigger the transaction to create the payment.
-
Cancelling with Reputation after the Funding step resulted in the following, you should still be able to cancel after funding and the cancel pill should appear after the funding step.
CzarekDryl
force-pushed
the
feat/15886093-reputation-cancel
branch
from
255d868 to
b0a6149
Compare
There was a problem hiding this comment.
Nice start of this, I'm able to get through the entire flow.
There is an issue that if you reject the first motion and start another one, when the cancel step reappears it loads into the first motion rather than the staking stage of the second one, so it looks like it is in a broken state.
And I'm experiencing the same issue as Arren when attempting to cancel after the funding stage.
It also seemed odd that the cancel motion step completely disappears if you reject the motion, I think you should still be able to access the failed motions and the step should remain visible, especially if there are unclaimed stakes / rewards.
CzarekDryl
force-pushed
the
feat/15886093-reputation-cancel
branch
2 times, most recently
from
957a3a5 to
d01637e
Compare
There was a problem hiding this comment.
@CzarekDryl Thank you for your hard work on this. I noticed a few things while testing:
-
Canceling with Permissions after the Review stage resulted in the first incomplete action timeline, and if after funding, the second screenshot.
-
Can these have a hover state when there are multiple cancellation requests:
-
In this case I created a motion to cancel the action, then while voting I used Permissions to cancel it right away. It cancelled correctly but stayed in this view. Is it possible to remove all the following steps in the timeline and show a message in this case. Something like this Figma link.
-
I saw this error in console when revealing a vote:
-
It seems if there are any still running motions then the extra steps in the timeline persists, despite a motion already passing and being finalized and the action being cancelled. It should remove all steps if the action has been cancelled.
-
Claiming staked tokens shows this broken pending button, however, is it possible to auto return this stakes when cancelling?
-
Cancelling with permissions with no penalty showed this timeline, it should have the step label be "Cancel", as the stakes have already be returned, the text should be "The action was canceled and cannot be executed."
Figma link showing expected designs
- While testing, I noticed that we a missing the indicator as to whether the cancellation motion is penalising or not, can we add a pill to a motion item like this design? Figma link.
|
@arrenv Everything should be fixed now, please check it again. Penalty badge added to cancel request item. 
|
There was a problem hiding this comment.
Thanks for your work on this PR @CzarekDryl, generally most of the functionality is there, but I spotted a few issues.
There appears to be regression around how cancel in review step is displayed:
master for comparison:
The note in the Cancel modal should display the actual role held by the user instead of hardcoded Payer:
I was able to get into a broken state by quickly funding or releasing a payment while the funding motion was loading. The UI should be disabled during that period:
Screen.Recording.2024-12-11.at.23.02.55.mov
When cancelling a staked expenditure with penalise option, the modal gets stuck in Pending. There are errors in the console which are most likely due to the fact the UI is trying to reclaim stake (which has been forfeited) - I commented on where I think it happens in code.
The same thing happens with cancelling via motions, on motion finalization.
Penalise badge has a border that is not in the designs:
- Figma
- UI
Other flows tested which mostly work well:
Cancelling via permissions, non-staked expenditure:
- At Funding:
- At Release:
Cancelling via reputation, non-staked expenditure:
Cancelling via permissions, staked:
Cancelling via reputation, staked expenditure:
| if (penalise) { | ||
| await cancelStakedExpenditureViaMotion(stakedMotionPayload); | ||
| } else { | ||
| await cancelExpenditureViaMotion(motionPayload); | ||
| } |
There was a problem hiding this comment.
Which saga to use should not depend on penalise but rather on the expenditure type - if staked, cancelStakedExpenditureViaMotion should always be used. You can pass the penalise as shouldPunish param to the saga.
| } else if (penalise) { | ||
| await cancelStakedExpenditure(stakedPayload); | ||
| await reclaimExpenditureStake(reclaimPayload); | ||
| } else { |
There was a problem hiding this comment.
Similarly here, cancelStakedExpenditure should always be used for staked expenditure regardless of penalise value.
Also, if creator was penalised, they lost their stake, so there is nothing to claim.
| @@ -0,0 +1,80 @@ | |||
| import clsx from 'clsx'; | |||
There was a problem hiding this comment.
Is there a way to reuse code from similar components? Composition via props could be used for elements specific to this type of action, e.g. the penalise badges.
There was a problem hiding this comment.
@jakubcolony Can we do this in other issue and refactor all request items?
|
@jakubcolony Everything should be fixed, please check it :) |
There was a problem hiding this comment.
Nice one @CzarekDryl, it is looking and working really well.
I did still run into a couple of things, not sure if they should be tackled in this PR.
-
If created using staking, and when finalizing a cancelation or using permissions to cancel I received this transaction error in the console.
-
If cancelling with Permissions and penalising, or not, it does not show which option was selected in the card, where it has been added when you use Reputation.
-
I received this error in the console when trying to stake, despite having more then enough tokens to stake.
|
@arrenv I've added badge while using permissions and it's in finalize pill And I'm not able to replicate console errors: Screen.Recording.2024-12-17.at.13.44.48.movScreen.Recording.2024-12-13.at.14.47.47.mov |
|
I have just seen this @CzarekDryl, is this related to this PR?
|
CzarekDryl
force-pushed
the
feat/15886093-reputation-cancel
branch
from
22e07b0 to
6263a83
Compare
|
@arrenv I've checked and all descriptions work fine for me |
There was a problem hiding this comment.
Thanks for another round of changes @CzarekDryl, mostly looking good but I'm still seeing some issues with staked expenditure.
When cancelling using permissions, the cards are different depending on whether the creator was punished or not.
If punished, no label is displayed informing of this:
If not punished, an extra Finalize step is added to the stepper:
It appears that the UI is still trying to reclaim the stake even though the payment creator was penalised, leading to some errors in the console:
I am still able to click around the UI while the cancel modal is in pending state. That's how I got the "Paid" label as well as a cancellation request in progress:
Otherwise, things are working well:
CzarekDryl
force-pushed
the
feat/15886093-reputation-cancel
branch
from
6263a83 to
0b9ce27
Compare
|
@jakubcolony All fixed |
There was a problem hiding this comment.
@CzarekDryl Thank you for the updates. Going through Jakub's issued raised, mostly look resolved from what I could tell, except for the extra Finalize step, with some additional issues.
-
I created an Advanced payment using Staking, I confirmed the details, then I cancelled and did not penalize using Permissions. The result was both a "Cancel" and a "Finalize" stepper. Which both had different information, one saying "Penalised", and the other saying "No penalty".
There should not be an extra Finalize step, the pill should only be "No penalty", the stake amount should be included in the cancel step along with the label "Cancelled" as opposed to "Created".
CzarekDryl
force-pushed
the
feat/15886093-reputation-cancel
branch
from
8747f5f to
4eae349
Compare
|
@arrenv Now reclaim step is showing only when user is penalised: 
And here is version for not penalised: 
|
There was a problem hiding this comment.
Doesn't seem far off, thanks for your continued work on this.
The note should not be shown when cancelling a payment made using permissions (non-staked):
The spacing between labels in the action info box is inconsistent:
I think the finalize step should not be shown in both cases (penalised and not penalised) as all the details are already in the cancel step. But I'll let @arrenv confirm that.
Cancelling staked payment while in review should automatically reclaim the stake (this is implemented on the contracts level). However it seems like the wrong method is called by the cancel saga, because it wasn't provided with stakedExpenditureAddress:
| @@ -124,14 +79,19 @@ const ActionWithPermissionsInfo: FC<ActionWithPermissionsInfoProps> = ({ | |||
| </div> | |||
| </> | |||
| )} | |||
| {additionlInfo && additionlInfo} | |||
|
@CzarekDryl & @jakubcolony, yes, when an action has been cancelled, then there should be no finalize step. The person who originally staked the advanced payment should receive their stake back when the action has been cancelled. The members who staked on the cancel motion, should still be able to claim their stakes back in the prior step within the cancel step, rather then there being an additional Finalize step. |
|
@jakubcolony Spacing between heading are different because of items height, between items they are the same. Now there is only cancel step: |
CzarekDryl
force-pushed
the
feat/15886093-reputation-cancel
branch
from
cb9b24a to
c3455ef
Compare
There was a problem hiding this comment.
All the previous issues have been resolved, there's just one problem I run into - when cancelling a staked payment with penalising creator, all the steps collapse and it's not possible to select any:
Screen.Recording.2025-01-26.at.22.11.49.mov
Please have a look at the failing Github check:
The note is no longer displayed when cancelling a non-staked payment ✅
Stake is reclaimed when cancelling a staked payment in review stage ✅
Screen.Recording.2025-01-26.at.21.55.32.mov
There is no extra step when cancelling a staked payment ✅
Nice work on this so far, let's get it over the line!
fix after rebase fix: translations, duplicated steps fix: cancel step fixes after rebase fix: auto claim, stepper Feat: Add willPunishExpenditureStaker field to colony motion feat: add penalty badge fix: pending buttons, penalise saga fix: types fix: permission badge fix: reclaim, badge fixes after rebase fixes after rebase fix: cancel and reclaim step fix: remove finalize step from cancel
adam-strzelec
force-pushed
the
feat/15886093-reputation-cancel
branch
from
01019e0 to
d6e8f88
Compare
There was a problem hiding this comment.
This is a huge amount of work, so great job so far!
There are unfortunately a few issues though, some may be out of scope, but I'll list them here regardless.
All cancellation motions have a "No Penalty" pill, even when the "Penalty" option selected.
Screen.Recording.2025-02-05.at.10.07.182.mov
If you have an active funding motion when cancelling, the widget becomes unusable.
Screen.Recording.2025-02-05.at.10.05.04.mov
If you have an active funding motion, then attempt to cancel via reputation, the widget becomes unusable.
Screen.Recording.2025-02-05.at.10.14.46.mov
Similar issues with an active release motion.
Screen.Recording.2025-02-05.at.10.17.50.mov
Otherwise things are mostly working great, again there is a lot of good work here!
I've also left a few comments where translation strings have been missed.
| The payment creator will keep their full stake and | ||
| reputation |
There was a problem hiding this comment.
Should use a translation string.
| The payment creator will lose their full stake and | ||
| the relative amount of reputation. Penalised funds | ||
| are burned. |
|
|
||
| const handleFundExpenditure = async () => { | ||
| const handleFundExpenditure = async ({ decisionMethod, penalise }) => { |
There was a problem hiding this comment.
handleCancelExpenditure ?
| onChange={onChange} | ||
| hasError={!!error} | ||
| > | ||
| No, don't penalise |
There was a problem hiding this comment.
Translation string here.
| onChange={onChange} | ||
| hasError={!!error} | ||
| > | ||
| Yes, penalise |
| </ul> | ||
| {!!error && ( | ||
| <p className="mt-1 text-sm text-negative-400"> | ||
| A penalised option is required |
There was a problem hiding this comment.
This file is getting huge, if there is anything that can be done to separate it up that would help make it more readable. (Can be left for a later PR if not feasible now)
| @@ -16,6 +17,6 @@ export interface PaymentBuilderWidgetProps { | |||
| export interface FinalizeStepProps { | |||
| expenditure: Expenditure | undefined | null; | |||
| expectedStepKey: ExpenditureStep | null; | |||
| expenditureStep: ExpenditureStep | null; | |||
| expenditureStep: string; | |||
There was a problem hiding this comment.
Why not ExpenditureStep?
Description
This PR introduces options for penalizing or not penalizing the cancellation of staked payments. It also implements a reputation method for canceling after the locked state and the handling of multiple cancellation requests.
Testing
Main issue - #2256