Skip to content
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

Better Audit UI #109

Merged
merged 16 commits into from
Jul 27, 2023
Merged

Better Audit UI #109

merged 16 commits into from
Jul 27, 2023

Conversation

devpanther
Copy link
Contributor

@devpanther devpanther commented Jul 20, 2023

resolves #100

  • Remove the "Etherscan API Key" text input. ✅

  • Remove the "RPC URL" text input. ✅

  • Remove the "Owner name" field. ✅

  • Refactor the "Repository name" field to "Project URLs ✅

Modify the analytics table to contain the following columns:

  • organization name/repo name (which is a URL to the repo under the hood) ✅
  • bounty hunter nickname who solved an issue ✅
  • amount ✅
  • on-chain payout tx hash ✅

Add a search text field above all of the table columns for filtering results ✅

I also added a sort for the Amount

@devpanther devpanther requested a review from 0x4007 as a code owner July 20, 2023 06:29
@github-actions
Copy link
Contributor

@devpanther
Copy link
Contributor Author

Oops, Github disabled my PAT because its on this PR.. so which means we might have to find a way to hardcode the PAT

@github-actions
Copy link
Contributor

@github-actions
Copy link
Contributor

@github-actions
Copy link
Contributor

@github-actions
Copy link
Contributor

@devpanther
Copy link
Contributor Author

@rndquu ready for review

@rndquu rndquu self-requested a review July 20, 2023 22:30
@rndquu
Copy link
Member

rndquu commented Jul 20, 2023

@rndquu ready for review

Thank you, I'll check it today

@github-actions
Copy link
Contributor

@rndquu
Copy link
Member

rndquu commented Jul 21, 2023

@devpanther

From the original issue: Check that the "Github Personal Access Token" input field is necessary. As far as I remember we need this PAT for some of the github endpoints but I'm not sure, pls check

So, is github PAT necessary for calling github APIs? Did you try calling github APIs with an empty PAT?

@devpanther
Copy link
Contributor Author

devpanther commented Jul 21, 2023

So, is github PAT necessary for calling github APIs? Did you try calling github APIs with an empty PAT?

Yea, it works but gets crazy late limited without the PAT.

It would be a problem to fetch comments because of the constant ban on IP, so we def need it

Copy link
Member

@rndquu rndquu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Github automatically invalidates all PATs this it finds in repositories. Pls bring back the "Github PAT" text field and read PAT from that field. No need to hardcode it. Besides calling the audit script with the single hardcoded PAT increases chances of hitting rate limits.
Screenshot 2023-07-21 at 12 33 33 3. If you run the audit script and wait for it to finish, and then click the "Get report" button again then issues appear with empty tx URLs Screenshot 2023-07-21 at 12 42 04

@devpanther
Copy link
Contributor Author

@rndquu Fixed these, try to test again

@github-actions
Copy link
Contributor

Copy link
Member

@rndquu rndquu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loader icon is missing

How to reproduce:

  1. Disable cache
  2. Run the script and wait for it to finish
  3. Click the "Get report" button

On step 3 the loader icon in the "Get report" button is going to be missed although the audit script will be running (if you check the console)

@devpanther
Copy link
Contributor Author

devpanther commented Jul 22, 2023

@devpanther

Why the following issues are missing in the audit table when running for the https://github.com/ubiquity/pay.ubq.fi repo?

I just checked, the permits were never used so no txHash to tag along with the issues... most of them were solved by pavlovcik and if you check the permits, they're still valid

I can also confirm I've not withdrawn this #86

@github-actions
Copy link
Contributor

@devpanther
Copy link
Contributor Author

Loader icon is missing

Traced the problem and fixed

@rndquu
Copy link
Member

rndquu commented Jul 23, 2023

@devpanther
Why the following issues are missing in the audit table when running for the https://github.com/ubiquity/pay.ubq.fi repo?

I just checked, the permits were never used so no txHash to tag along with the issues... most of them were solved by pavlovcik and if you check the permits, they're still valid

I can also confirm I've not withdrawn this #86

@pavlovcik Do we need to display closed issues with unclaimed permits in the audit result table?

Right now only issues with claimed permits are shown

@devpanther
Copy link
Contributor Author

devpanther commented Jul 23, 2023

I think this would be fixed better by the time we implement claim all (making it easier to claim permits)

It also depends on the purpose of the audit page, is it to show issues solved/progress or to display issues solved and money spent.

Personally, I think if a permit isn't claimed then there's no need for an audit, it'd just be the same as going through github to view the closed issues if a tx hash isn't attached on the audit

@0x4007
Copy link
Member

0x4007 commented Jul 25, 2023

@devpanther

Why the following issues are missing in the audit table when running for the https://github.com/ubiquity/pay.ubq.fi repo?

I just checked, the permits were never used so no txHash to tag along with the issues... most of them were solved by pavlovcik and if you check the permits, they're still valid

I can also confirm I've not withdrawn this #86

@pavlovcik Do we need to display closed issues with unclaimed permits in the audit result table?

Right now only issues with claimed permits are shown

That's a good question I'm not sure off hand. I'll know much better after using it.

However if there is a way to pull in all the claims information (claimed and unclaimed) to the audit page, and then later add filters to display the useful information to the auditor, that approach seems to make the most sense to me.

@devpanther
Copy link
Contributor Author

devpanther commented Jul 25, 2023

However if there is a way to pull in all the claims information (claimed and unclaimed) to the audit page, and then later add filters to display the useful information to the auditor, that approach seems to make the most sense to me.

That'd be hundreds of issues and It'd take a lot of time at the current functionality to get everything and then filter out the claimed.. The reason it takes a shorter time now is it prioritises the txHash and does its search from there.

Maybe by the time we move over to fetching from DB and its a lot more memory intensive.. it'd be okay to mix issues without a txHash and then add a checkbox to hide/show

besides making it show unclaimed right now, is changing most of the underlying code and rewriting the queue functionality (not in scope for this issue)

@rndquu
Copy link
Member

rndquu commented Jul 26, 2023

@devpanther

Why the following issues are missing in the audit table when running for the https://github.com/ubiquity/pay.ubq.fi repo?

I just checked, the permits were never used so no txHash to tag along with the issues... most of them were solved by pavlovcik and if you check the permits, they're still valid

I can also confirm I've not withdrawn this #86

@pavlovcik Do we need to display closed issues with unclaimed permits in the audit result table?
Right now only issues with claimed permits are shown

That's a good question I'm not sure off hand. I'll know much better after using it.

However if there is a way to pull in all the claims information (claimed and unclaimed) to the audit page, and then later add filters to display the useful information to the auditor, that approach seems to make the most sense to me.

Created a separate issue for displaying issues with unclaimed permits ubiquity/audit.ubq.fi#5

This PR looks good, pls review

@0x4007 0x4007 merged commit ea8c586 into ubiquity:development Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

audit dApp: Better UI
3 participants