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.

Sign up for GitHub

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

Jump to bottom

Feat/api voting list #3824

Merged
merged 20 commits into from
Aug 18, 2023
Merged

Feat/api voting list #3824

merged 20 commits into from
Aug 18, 2023

Conversation

agualis
Copy link
Collaborator

@agualis agualis commented Jul 31, 2023 •
edited
Loading

Description

Migrates vebal voting list to use new API endpoint VeBalGetVotingList (that is already deployed in production)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Dependency changes
  • Code refactor / cleanup
  • Documentation or wording changes
  • Other

How should this be tested?

Please provide instructions so we can test. Please also list any relevant details for your test configuration.

  • Test A
  • Test B

Visual context

Please provide any relevant visual context for UI changes or additions. This could be static screenshots or a loom screencast.

Checklist:

  • I have performed a self-review of my own code
  • I have requested at least 1 review (If the PR is significant enough, use best judgement here)
  • I have commented my code where relevant, particularly in hard-to-understand areas
  • If package-lock.json has changes, it was intentional.
  • The base of this PR is master if hotfix, develop if not

@agualis agualis self-assigned this Jul 31, 2023
@vercel
Copy link

vercel bot commented Jul 31, 2023 •
edited
Loading

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

Name Status Preview Comments Updated (UTC)
beta-app-v2 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 17, 2023 3:32pm
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
app-v2 ⬜️ Ignored (Inspect) Visit Preview Aug 17, 2023 3:32pm

@agualis agualis marked this pull request as ready for review August 3, 2023 13:52
timjrobinson
timjrobinson approved these changes Aug 9, 2023
View reviewed changes
Copy link
Contributor

@timjrobinson timjrobinson left a comment

Choose a reason for hiding this comment

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

LGTM. It does take a few seconds to load becuase of a subgraph query to fetch all pools on the veBAL page. Is that query needed? Does the veBAL table need to wait for it before showing?

src/components/contextual/pages/vebal/LMVoting/GaugeVoteModal.vue Outdated
const gaugeVoteWeightNormalized = scale(props.gauge.votesNextPeriod, -18);
const gaugeVoteWeightNormalized = scale(props.pool.votesNextPeriod, -18);
if (isVeBalGauge.value) {
return gaugeVoteWeightNormalized.gte(bnum('0.1'));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this hard coded here instead of dynamically pulled like the rest? Can it be put into a const variable rather than a magic number.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I decided to keep it as I found it cause the author probably wanted to explicitly enforce this concrete rule.
I will extract the magic number to a const just in case we are missing something.

@agualis
Copy link
Collaborator Author

agualis commented Aug 9, 2023

LGTM. It does take a few seconds to load becuase of a subgraph query to fetch all pools on the veBAL page. Is that query needed? Does the veBAL table need to wait for it before showing?

That AllPools query is triggered by balancer-sdk dependency (from PoolCacher) that we use from many different places but it is not blocking.

As we discussed in slack the main blocker here would be the decorator that wraps pools/gauges with voting data from onchain multicalls:

frontend-v2/src/services/balancer/gauges/gauge-controller.decorator.ts

Line 73 in 478fccf

async decorateWithVotes(

garethfuller
garethfuller approved these changes Aug 10, 2023
View reviewed changes
Copy link
Collaborator

@garethfuller garethfuller left a comment

Choose a reason for hiding this comment

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

Let's get Zekraken to do a round of testing just on voting to make sure that's all working correctly, but he can do that on beta.

@agualis agualis marked this pull request as draft August 10, 2023 11:26
@agualis agualis changed the title Feat/api voting list Feat/api voting list [don't merge until latest API changes are in production] Aug 10, 2023
@agualis
Copy link
Collaborator Author

agualis commented Aug 10, 2023

Let's get Zekraken to do a round of testing just on voting to make sure that's all working correctly, but he can do that on beta.

The code is ready in canary but Franz is testing some new infra in production so we need to wait for him to finish. I will merge then.

@agualis agualis changed the title Feat/api voting list [don't merge until latest API changes are in production] Feat/api voting list Aug 18, 2023
@agualis agualis marked this pull request as ready for review August 18, 2023 07:04
@agualis agualis merged commit d713f41 into develop Aug 18, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garethfuller garethfuller garethfuller approved these changes

@timjrobinson timjrobinson timjrobinson approved these changes

@alter-eggo alter-eggo Awaiting requested review from alter-eggo

Assignees

@agualis agualis

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@agualis @timjrobinson @garethfuller