Closed rcullito closed 6 years ago
A few thoughts:
winner_login
? Would we need to set winner_login
at all? Maybe an alternative would be to just create the payout transaction with the owner's address?revoked_ts
with a timestamp of revocation. I see you did that to some extent in the commit_sha
column but this might warrant an actual column. An interesting implication around this is also that multiple people may pay into a bounty. Ideally each of them would get their funds back but I'm not sure how feasible this is at this point.
@martinklepsch thanks for reviewing this!
Do we need to file a fake claim? Could we not simply set winner_login?
If we want this bounty to show up in the "manage payouts" dashboard with the existing logic, then we do need a claim in place. (Or conversely, we could update the logic for how an entity shows up in the manage payouts dashboard 😄) I totally agree that there is a lot of "fat" to be trimmed here for the revocation scenario, but I wanted to witness a few full end to end payouts before starting to cut out the unnecessary parts.
It's important that we track if a bounty has been revoked, consider adding a column revoked_ts with a timestamp of revocation.
👍 yepp, makes sense!
multiple people may pay into a bounty
good point! this issue, and a few others like it are causing me to try and research events emitted by the smart contracts themselves, so our source of truth is not derived solely from postgres, which is just the aggregate balances without a breakdown by contributor. I've been checking out this library (https://github.com/district0x/re-frame-web3-fx#web3watch-events) as it seems like it is easier to respond to events via the browser than constantly polling over json-rpc on the backend, but it's totally possible that I'm missing some part of the current sob codebase and we may already be doing more in this area than I understand cc @siphiuel
cc @3esmit re the multiple people paying into a contract to provide clarity how this will work with the revised smart contract.
@rcullito we will definitely need to adapt the UI to some extent so that revocation is clearly communicated. Not sure what this will look like in the end but it might make sense to think about it in terms of a single API request that receives an issue_id
as argument.
@EugeOrtiz We will need some way to have secondary actions on these bounty cards (both, big and square ones) — maybe 3 dots in the top right corner or something similar to cards on the GitHub project boards.
@martinklepsch thanks for mulling it over and agreed that an api w/ issue-id makes the most sense from a UX perspective (as well as the clear intention to revoke).
The main bottleneck for me is ensuring that the confirm-payout then works for an address(addresses) other than the repo owner as trials so far in that area have been failing at the contract level, although I'm trying to work through it.
cc @siphiuel @martinklepsch I still have a couple of UX features to continue improving after chatting with Euge, but was hoping to get your feedback on how the overall implementation looks before fiddling with any further changes :)
What we decided was easiest was the scenario of the user hitting a single button "revoke" to kick off the various steps. Work is then dispatched from a reframe handler to a route that polls for confirmation from the commiteth bot, and on success dispatches a confirm payout event that prompts for confirmation from metamask.
Create a bounty and add funds to it. Only bounties with a balance greater than 0 will be eligible to be revoked. (Please keep in mind that balances are updated by a scheduler job so they may take some time to updated in the SOB UI.)
Once the funds are added to the bounty, navigate to the Manage Payouts Screen in Status Open Bounty and visit the "Unclaimed Bounties" section. There should be a card there containing the issue title of the bounty you just created. In the lower right hand corner will be three dots. Click on those to bring up the "Revoke Bounty" button. Then click on it :) A green banner should now appear stating that your transaction has been sent. *note: In order for a bounty to be revoked, the refunding must be confirmed by both the commiteth bot account and the local metamask account, and at this stage in the process the first of these has been initiated. You'll have to wait a bit for the bot confirmation to complete, and the link in the green banner will direct you to the contract address on etherscan so that you can watch the first of the two confirm transactions change from pending to succeeded (hopefully).
Then it will take a minute for the scheduler job running in the background to pick up the change, and with one confirmation in hand, prompt you – the user – to confirm the transaction via metamask. At this stage if you reject the transaction, the revocation will fail. Submitting via metamask will leave the revocation in its pending state, but you can now check the status of this second transaction by navigating via the banner link to the etherscan contract address (and watch for the second confirmation transaction event to move from pending to succeeded).
Now, finally, there is no more action to take from the UI and the scheduler thread should pick up confirmation of the second transaction. When it does, the green banner will go away, indicating that the user has left the pending state, and your bounty "card" will move from the "Unclaimed" section at the top to the "Paid out" section at the bottom.
In summary: a.) click the three dots and then revoke b.) wait, optionally check the etherscan link via the green banner c.) confirm the transaction via metamask d.) wait, watch for the banner to disappear and the bounty to move from "unclaimed" to "paid out".
@rcullito when i click 3 dots to revoke bounty - all cards show this option (i am using Chrome)
also clicking the button does nothing.
@asemiankevich thank you for looking at it so promptly! I hadn't been experiencing that but will double check now on my local branch to see if I introduced a regression that might be causing this.
@asemiankevich I've pushed out a fix for the multiple popups (nice catch on your part 🙏 ). In terms of what happens next, a green pending banner will be displayed at the top of the page. So I think it would be just out of view in the screenshot you posted. Although I can certainly examine if that's not happening for you on this most recent version.
Steps: Requirements: GH account is whitelisted, signed app, test application is added to repo;
"Transaction sent. Refund is pending until the transaction is completed. Check its status here. " - is not visible for user unless he will scroll to the top
The message is visible and placed in screen area (something like popup window)
Screen:
Wallet: MetaMask OS: Mac OS High Sierra Browser: Chrome 64
Steps: Requirements: GH account is whitelisted, signed app, test application is added to repo;
bounty is displayed in "Paid" section
Separate section for revoked bounties
Screen:
Wallet: MetaMask OS: Mac OS High Sierra Browser: Chrome 64
Steps: Requirements: GH account is whitelisted, signed app, test application is added to repo; 1) create issue with bounty, fund it 2) go to Manage Payouts 3) click on three dots -> "Revoke". Pay attention to other unclaimed and funded bounties. 4) refresh the page. Pay attention to revoked bounty.
3) "Revoke" option disappears on all bounties 4) "Revoke" option appear everywhere, even on already revoked bounty
3) "Revoke" option disappears on revoked bounty only 4) No changes after reload
Video: http://take.ms/9tzIm Wallet: MetaMask OS: Mac OS High Sierra Browser: Chrome 64
Monosnap screenshot toolMonosnap — the best tool to take & share your screenshots.
Steps: Requirements: GH account is whitelisted, signed app, test application is added to repo; 1) create issue with bounty, fund it 2) go to Manage Payouts 3) click on three dots -> "Revoke". Pay attention to message "Transaction sent. Refund is pending until the transaction is completed. Check its status here."
Should be obvious that for revoking bounty (I think the good idea is even displayed id or issue title of bounty in message) user SHOULD confirm transaction after one or several (depending on tokens variety in bounty) transactions will be completed. IMO message should be more informative, i.e.: Transaction(s) for revoking #X issue are sent. Refund is pending until the transaction(s) are completed (check its status here), then final transaction will be sent to your MetaMask wallet. Confirm it to finish revocation.
Wallet: MetaMask OS: Mac OS High Sierra Browser: Chrome 64
Steps: Requirements: GH account is whitelisted, signed app, test application is added to repo; 1) create an issue with bounty, fund it 2) go to Manage Payouts 3) click on three dots -> "Revoke". 4) in MetaMask reject transaction 5) click on three dots -> "Revoke" again
bot transaction is generated; no transaction to confirm in MetaMask;
after revocation failed "Revoke" option shouldn't be available because transaction for payment will not appear anymore
Wallet: MetaMask OS: Mac OS High Sierra Browser: Chrome 64
I couldn't detect specific steps to reproduce, faced with this behavior only once. I can see that all transactions are successful, but bounty is still placed in Unclaimed section, GH comment is not updated (after revocation 2 hours passed)
revoked bounty in "Unclaimed"
bounty is displayed along with other revoked bounties Issue: https://github.com/TestingEnvENV/TestEnvAtt5_1/issues/254 Wallet: MetaMask OS: Mac OS High Sierra Browser: Chrome 64 logs: log.log
@rcullito just to summarize all issues in one place:
@churik thanks again for the feedback, I think I have a pretty good feel for the issues you have raised.
Individual comments & progress are inline above among the checkboxes, but I had two main questions:
1.) do you know the cause of the failure during your testing? was it rejecting via metamask? or navigating away from the dashboard? or something completely different? 2.) If not in the "Paid" section, where were you expecting these bounties to go, into a dedicated revoked section or not be displayed at all? @EugeOrtiz this is also something I had meant to circle up with you on but... well, here we are :)
Thanks both! - Rob
@rcullito thanks! 1.) If it is about 'Sometimes issue stays in Unclaimed bounties after revocation' - unfortunately, no, I couldn't find something specific. I confirmed transaction along with other through MetaMask. And if I checked transactions TestingEnvENV/TestEnvAtt5_1#254 - everything looks like it should be. But bounty stuck in "Unclaimed" section. 2.)Yes, I believe that it should be separate section. @EugeOrtiz can you help with it?
@churik feedback addressed in line above ^^. pretty happy with where this landed and should be good for another round of testing 🙏
Steps: Requirements: GH account is whitelisted, signed app, test application is added to repo; 1) create issue with bounty, fund it 2) go to Manage Payouts 3) click on three dots -> "Revoke". 4) wait until transaction is completed 5) pay attention to "Paid out to" value in "Paid out" section 6) open issue on GitHub, pay attention to bot comment "Paid out" value
values are different
values should be the same in GH comment and in "Paid out" SOB section
Wallet: MetaMask OS: Mac OS High Sierra Browser: Chrome 64
Steps: Requirements: GH account is whitelisted, signed app, test application is added to repo; 1) create an issue with bounty, fund it 2) go to Manage Payouts 3) click on three dots -> "Revoke". 4) go to My payment details 5) set new address and click on "Save" 6) repeat steps 2-3
value is updated onlya fter manual reload
value is updated automatically
Wallet: MetaMask OS: Mac OS High Sierra Browser: Chrome 64 Video: http://take.ms/5oCUD
With last changes issue appered that transaction is not triggered when bot transaction is already completed, so revocation is broken.
Steps: Requirements: GH account is whitelisted, signed app, test application is added to repo; 1) create an issue with bounty, fund it 2) go to Manage Payouts 3) click on three dots -> "Revoke". 4) wait until transaction is completed 5) wait for 10 min more (for MetaMask confirmation window)
no confirmation window in MetaMask
confirmation window appears once bot transaction is completed Issues: https://github.com/TestingEnvENV/TestEnvAtt5_1/issues/264 https://github.com/TestingEnvENV/TestEnvAtt5_1/issues/263 https://github.com/TestingEnvENV/TestEnvAtt5_1/issues/261
Wallet: MetaMask OS: Mac OS High Sierra Browser: Chrome 64 Log: log1.log
thanks tetiana!
in summary:
@rcullito thanks for your work!
unfortunately, I still don’t see any transactions in MetaMask for revocation confirming. So I’m still blocked with testing https://github.com/status-im/open-bounty/pull/430. I don’t know where exactly issue could be - probably it is even not our part, but 1) I didn’t upgrade MetaMask 2) all that I have done - change account in My payment details several times (I did it several times before last changes in PR) All that I’ve got now - bot transaction is completed, but no more transaction is triggered through MetaMask so revocation can’t proceed. For first time issue reproduced during testing iteration 16-17.05.18. Before these changes, this behavior wasn't reproducible
Also, please take a look into this comment:
"Revoke" option disappears and appears needlessly (RC) - the intended behavior is that the option will not be visible during a revocation. This is the behavior I have been experiencing on successful revokes. Although I'll keep an eye on this while addressing your other points about funky state during failed revocations. @churik: I've experiencing behavior when issue state and availability of "Revoke" button change when you manually updated the page. I don't think it is OK (video: http://take.ms/vIGbH)
I'm still experiencing with this behavior.
@churik Ah ok, the video helps! Thanks for that. The difference in our experiences I believe is that I am not refreshing during my own process, but that is a fair point to bring up. I'll push some changes to persist our app state for revocation into localStorage so that it is not as brittle.
@churik alright this should be pretty bullet proof at this point, resistant to page refresh, earthquakes... whatever else might come up lol.
Thank you for your help! This should be much more robust now if you are able to test again. Please let me know if you are not experiencing an improvement.
To summarize for now: 1) After updating payment address it is not updated automatically in the informational message - existing issue 2) Revoked bounties are not displayed separately on dashboard, but they don't affect on "Paid for 1 solved bounties"
Part of https://github.com/status-im/open-bounty/issues/284
The workflow right now is to hit a
revoke
button on the manage payouts page which triggers a series of steps which are better chronicled here: https://realtimeboard.com/app/board/o9J_kz0D-rM=/