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

fix: fix active treasury proposals indicator #250

Merged

Conversation

rickstaa
Copy link
Contributor

@rickstaa rickstaa commented Dec 27, 2023

This commit ensures that the active treasury proposals indicator is working correctly. The changes in this proposal are also included in #247. This pull request was created so that it can already be merged while we still discuss the exact implementation of the Drawer treasury item in #247.

Before

image

After

image

This commit ensures that the active treasury proposals indicator is
working correctly.
Copy link

vercel bot commented Dec 27, 2023

@rickstaa is attempting to deploy a commit to the Livepeer Team on Vercel.

A member of the Team first needs to authorize it.

layouts/main.tsx Outdated Show resolved Hide resolved
@rickstaa
Copy link
Contributor Author

rickstaa commented Dec 27, 2023

@dob and @victorges, I've compiled a comprehensive report to bolster confidence in merging this pull request. The critical component of this pull request is derived from existing, peer-reviewed code in our repository:

const { data } = useTreasuryProposalsQuery({ pollInterval });
const proposals = useMemo(
() => data?.treasuryProposals.map((p) => parseProposalText(p)),
[data?.treasuryProposals]
);

Utilizing the useTreasuryProposalsQuery hook aligns seamlessly with the already reviewed code, ensuring stable integration. I've shared the detailed testing process for additional code validation below.

Detailed Test Report

Execution Steps for Testing

  1. Cloned the explorer repository for a baseline environment.
  2. I installed all JavaScript dependencies to mirror the production setup.
  3. Configured the necessary environment variables in the .env file for accurate simulation.
  4. Launched the explorer locally using the yarn dev command, creating a representative testing environment.

Comprehensive Testing Checklist

  • Initial Functionality Test: Confirmed the code's operational integrity without any apparent issues.
  • Indicator Accuracy Verification: Ensured the indicator reliably reflects the correct values.
  • Resilience Check for currentRound?.id: Validated that the code remains stable when currentRound?.id is undefined.
  • Stability Test for proposalsData?.treasuryProposals: Assured that undefined proposalsData?.treasuryProposals doesn't cause crashes.
  • Empty p.voteEnd Handling: Tested for the scenario where p.voteEnd is missing. Confirmed that in such cases, it defaults to 0, leading to totalActiveProposals being 0 without causing system instability.
  • totalActiveProposals Undefined Scenario: Verified the code's robustness in instances where totalActiveProposals is undefined.

Copy link
Member

@victorges victorges left a comment

Choose a reason for hiding this comment

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

Neat!

layouts/main.tsx Outdated Show resolved Hide resolved
layouts/main.tsx Outdated Show resolved Hide resolved
Copy link

vercel bot commented Dec 27, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
explorer-arbitrum-one ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 27, 2023 8:38pm

layouts/main.tsx Outdated Show resolved Hide resolved
This commit fixes the active treasury proposal check condition. It also
applies several code improvements.
@rickstaa rickstaa force-pushed the fix_treasury_active_proposals_indicator branch from ae73c7d to b3d868b Compare December 27, 2023 22:17
@rickstaa
Copy link
Contributor Author

@victorges, I've implemented the revisions you suggested. Could you take a look at commit b3d868b and let me know if there's anything else that requires modification?

Copy link
Member

@victorges victorges left a comment

Choose a reason for hiding this comment

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

LGTM!

@victorges victorges merged commit 2071617 into livepeer:main Dec 28, 2023
0 of 2 checks passed
@rickstaa rickstaa deleted the fix_treasury_active_proposals_indicator branch December 28, 2023 16:29
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.

None yet

2 participants