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

web: LQT integration #2041

Merged
merged 27 commits into from
Feb 19, 2025
Merged

web: LQT integration #2041

merged 27 commits into from
Feb 19, 2025

Conversation

TalDerei
Copy link
Contributor

@TalDerei TalDerei commented Feb 12, 2025

Description of Changes

end-to-end testing across view service, block processor, wasm, and indexedDB is challenging across separate PRs. This serves as a staging area to test and build on the code drafted in those individual draft PRs . Includes wasm, vitest, and testing against pcli for assurance.

references tracking issue: #2022 and pairs with prax-wallet/prax#292.


View Service:

  • LqtVotingNotes view service RPC retrieves the user's eligible voting notes for the current epoch, and performs nullifier queries against the nullifier set tracked by the nullifier state key in the funding service. Leverages existing IndexedDB storage method getNotesForVoting to retrieve eligible voting notes, and exposes an equivalent utility storage method in wasm.

  • TournamentVotes view service RPC retrieves the user vote for the associated incentivized asset in the current epoch.


Wasm:

  • necessary wasm plumbing to extend planner support for the ActionLiquidityTournamentVote action, which plans a transaction by directing all available voting power to a single asset.

IndexedDB:

  • LqtHistoricalVotes table records historical votes, and corresponding storage method for saving historical votes and rewards after voting and during syncing.

Action Views:

  • Implements visible and opaque action view and translators in deprecated UI library for testing purposes.

note: #2041 (comment) will be handled separately in #2065 as follow-up work.


eg. LQT vote and reward detection during syncing and saving to IndexedDB.

Screenshot 2025-02-13 at 1 52 46 PM

* view service: lqt voting notes scaffolding

* view service: tournament votes rpc wip
* indexedDB: lqt historical votes table

* workaround to normalize action cases
@TalDerei TalDerei self-assigned this Feb 12, 2025
Copy link

changeset-bot bot commented Feb 12, 2025

🦋 Changeset detected

Latest commit: 3ac2a91

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 15 packages
Name Type
@penumbra-zone/protobuf Major
@penumbra-zone/services Major
@penumbra-zone/storage Major
@penumbra-zone/types Major
@penumbra-zone/ui-deprecated Major
@penumbra-zone/perspective Major
@penumbra-zone/wasm Major
@penumbra-zone/ui Minor
minifront Patch
node-status Patch
@penumbra-zone/bech32m Major
@penumbra-zone/client Major
@penumbra-zone/getters Major
@penumbra-zone/crypto-web Major
@repo/tailwind-config Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@TalDerei TalDerei marked this pull request as draft February 12, 2025 02:48
@TalDerei TalDerei changed the title LQT: web integration web: LQT integration Feb 12, 2025
@TalDerei TalDerei marked this pull request as ready for review February 13, 2025 05:32
@TalDerei TalDerei requested review from erwanor, cronokirby and a team February 13, 2025 21:45
Copy link
Contributor

@cronokirby cronokirby left a comment

Choose a reason for hiding this comment

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

A few small suggested changes, otherwise this looks good ; I think we should modify the TPR to only accept a single denom for incentivization, which is the current behavior of the code anyways. I'll submit a PR to that effect in the main repo.

erwanor pushed a commit to penumbra-zone/penumbra that referenced this pull request Feb 14, 2025
## Describe your changes

Looking at the ongoing integration work in:
penumbra-zone/web#2041, the transaction planner
request for lqt votes can only make use of a single incentivized denom
anyways, which is what we want to support in the final ux, so having a
repeated field here that subsequently gets truncated to the first
provided value is superfluous. This change makes the field plain
instead.

This can be tested inside of that PR.

## Issue ticket number and link

## Checklist before requesting a review

- [x] I have added guiding text to explain how a reviewer should test
these changes.

- [x] If this code contains consensus-breaking changes, I have added the
"consensus-breaking" label. Otherwise, I declare my belief that there
are not consensus-breaking changes, for the following reason:

  > Affects clients only.
conorsch pushed a commit to penumbra-zone/penumbra that referenced this pull request Feb 14, 2025
## Describe your changes

Looking at the ongoing integration work in:
penumbra-zone/web#2041, the transaction planner
request for lqt votes can only make use of a single incentivized denom
anyways, which is what we want to support in the final ux, so having a
repeated field here that subsequently gets truncated to the first
provided value is superfluous. This change makes the field plain
instead.

This can be tested inside of that PR.

## Issue ticket number and link

## Checklist before requesting a review

- [x] I have added guiding text to explain how a reviewer should test
these changes.

- [x] If this code contains consensus-breaking changes, I have added the
"consensus-breaking" label. Otherwise, I declare my belief that there
are not consensus-breaking changes, for the following reason:

  > Affects clients only.
@TalDerei
Copy link
Contributor Author

@cronokirby where is the rolling over of notes to prevent nullifier linkability handled?

@cronokirby
Copy link
Contributor

cronokirby commented Feb 14, 2025

@cronokirby where is the rolling over of notes to prevent nullifier linkability handled?

https://github.com/penumbra-zone/penumbra/blob/protocol/lqt_branch/crates/bin/pcli/src/command/tx/lqt_vote.rs#L107-L121

We insert a spend of every delegator vote we use, creating a new note.

(In writing this, I noticed that we needlessly create outputs, instead of letting change work its magic, so in implementing this logic I think all that's need is just adding a spend for each note, and then setting the change address appropriately)

@TalDerei
Copy link
Contributor Author

TalDerei commented Feb 14, 2025

We insert a spend of every delegator vote we use, creating a new note.

doesn't that conflict against the messaging in https://discord.com/channels/824484045370818580/983778100821262398/998722753085591612?

@cronokirby
Copy link
Contributor

We insert a spend of every delegator vote we use, creating a new note.

doesn't that conflict against the messaging in https://discord.com/channels/824484045370818580/983778100821262398/998722753085591612?

The voting action itself doesn't require a spend ; however, it does reveal information about the note being spent, which allows linking the use of the same note for voting across multiple epochs. However, what you can do is, in addition to voting, actually spend the notes, consolidating all the voting power you have into a single note, which will now be unlinkable, and allow you to vote in the next round with a single action instead of several. This is an additional feature we can add when actually planning such actions.

This can be done at:

  1. the planner level (would advise against, it's useful to be able to not do this)
  2. at the planner request level (unsure if we should do here, probably not)
  3. when constructing the request (if we ever implement lqt voting in several places having to reimplement the logic might be a bit annoying, but a priori this seems like the best place to put the logic)

@TalDerei TalDerei requested a review from cronokirby February 17, 2025 07:38
Copy link
Contributor

@cronokirby cronokirby left a comment

Choose a reason for hiding this comment

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

I submitted a review in prax which is closely linked to this, but I think we need to shift things here to have multiple votes per epoch and transaction, to reflect what's possible, and that we need the view for the LQT action to show all the information present in the opaque case.

@TalDerei
Copy link
Contributor Author

TalDerei commented Feb 19, 2025

This is an additional feature we can add when actually planning such actions.

@cronokirby the only thing i'm punting on is nullifier likability by rolling over delegation notes (which we also don't currently do for delegator voting), which we can model after the chain upgrade when there's an actual consumer of the LQT work.

Copy link
Contributor

@cronokirby cronokirby 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 try and get CI green, but otherwise looks good!

Copy link
Contributor

github-actions bot commented Feb 19, 2025

Visit the preview URL for this PR (updated for commit 3ac2a91):

https://penumbra-ui-preview--pr2041-protocol-lqt-branch-mzjr9pdf.web.app

(expires Wed, 26 Feb 2025 22:14:02 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 709d729610ef7a6369b23f1cb2b820a60cc685b1

@TalDerei TalDerei merged commit 49ae3ab into main Feb 19, 2025
8 checks passed
@TalDerei TalDerei deleted the protocol/lqt_branch branch February 19, 2025 22:54
@TalDerei TalDerei mentioned this pull request Feb 20, 2025
8 tasks
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.

2 participants