Bug: cancel minion available for non-proposer

plor commented 2 years ago

Describe the bug On the proposal list view, an unsponsored minion transaction has a cancel button that is enabled for people other than the proposer. It shows correctly when you go to the detailed view.

To Reproduce Steps to reproduce the behavior:

Go to DAO and submit proposal (from minion)
Switch wallets to "non-proposer" address
See "Cancel Minion" button enabled (but fails if transaction tried)

Expected behavior

Non-proposer should see disabled "Cancel Minion" button (or no button at all).

Actual behavior

Button is enabled for non-proposer who cannot cancel.

Screenshots

This is the view from the non-proposer address

Application area

Where in the app is this happening?

FrontEnd (/proposals)

Additional context

plor commented 2 years ago

@jordang7 this is ready if you want to move on this, let me know if you have any questions.

jordang7 commented 2 years ago

@plor

Thanks, this should be easy to fix but I think I need a little clarification on who should be able to cancel the proposal before making any changes.

So when you submit the proposal from a minion, the minion address is referenced as the proposer, and the account who submitted the proposal is referenced as createdBy. Are you saying the account who submitted the proposal should be able to see the cancel button on the detail page?

Currently, there is conflicting logic in the codebase. I believe From the proposal view it's checking if the proposal.proposer === proposal.minionAddress, and the detail view is checking proposal.proposer === address. The former will allow anyone to see the cancel button since it will always be true. The latter will only show the cancel button to the minion account.

plor commented 2 years ago

Yes, there is confusion on this for sure, whether the proposal is coming from the minion or whether funds are being sent to the minion is the case that is being tested there I believe. In this case createdBy can be used to see whether the cancel should be allowed. The address that creates the minion proposal should be able to withdraw the proposal before it is sponsored.

This task is primarily focused at the list page and not the detail page because the logic seems flipped. But on the details page, it is disabled when it should not be. Again, let me know if that should be a separate issue.

jordang7 commented 2 years ago

I see that makes more sense now.

Yeah I believe the detail page should be a separate issue from this task in that case.

plor commented 2 years ago

Cool, I'll spin that out.

plor commented 2 years ago

@jordang7 your PR removes the button for non-proposers, but it also removes it for the address that should be able to cancel. To be clear on this, the member who proposes a proposal where a minion is doing something should be able to cancel that proposal before it is sponsored.

jordang7 commented 2 years ago

@plor So from what I've gathered from the logic for the proposal list view, it doesn't show a cancel button for the proposer in the first place. I added an extra conditional to the SponsorCard component which I don't think is shown to the proposer(only non-members or members who can sponsor), so I don't believe it should be changing the view for the proposer(approve button and no cancel button at all). Do you see the cancel button as the proposer without this change?

The component the proposer sees is the UnlockTokenCard, which doesn't have a cancel minion button. I can add a cancel button to that if wanted, but the issue stated no button was an excepted behavior as well so went with that. Let me know.

plor commented 2 years ago

Yes, it currently is working, it is just that the non-proposer can also cancel (or appears to).

(this is from the proposer's view)

jordang7 commented 2 years ago

Ah I see. Okay I will take another look and come up with a solution.

jordang7 commented 2 years ago

Updated PR should work for both cases now.

jordang7 commented 2 years ago

Realized my solution was wrong again after looking it over. I believe the cancel button displayed to the proposer should be the cancel minion button, not the `cancel proposal. Sorry about that, let me know if I've got it right this time.

plor commented 2 years ago

Sorry about the delay on merging this, I had some local env issues that was delaying some testing. I did find an issue with the logic that allows both buttons to show up for some proposals (see screenshot)