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

view service: Implement LQT rpcs #5062

Merged
merged 7 commits into from
Feb 6, 2025

Conversation

TalDerei
Copy link
Collaborator

@TalDerei TalDerei commented Feb 6, 2025

Describe your changes

defines and stubs LqtVotingNotes and TournamentVotes view service rpcs.

Issue ticket number and link

Checklist before requesting a review

  • I have added guiding text to explain how a reviewer should test these changes.

  • 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:

    LQT branch

@TalDerei TalDerei added the consensus-breaking breaking change to execution of on-chain data label Feb 6, 2025
@TalDerei TalDerei self-assigned this Feb 6, 2025
@TalDerei TalDerei changed the base branch from main to protocol/lqt_branch February 6, 2025 00:31
@TalDerei TalDerei marked this pull request as draft February 6, 2025 00:31
@TalDerei TalDerei marked this pull request as ready for review February 6, 2025 00:43
core.component.sct.v1.Nullifier nullifier = 2;
}

message LqtVotingNotesResponse {
Copy link
Member

Choose a reason for hiding this comment

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

That doesn't seem right, because this RPC is supposed to return SNRs that can be used to vote in the LQT

Copy link
Collaborator Author

@TalDerei TalDerei Feb 6, 2025

Choose a reason for hiding this comment

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

right makes sense, thinking the request should take the epoch_index and address_index, and response should just be list of all eligible SNRs that are eligible and haven't been used to vote yet in current LQT period?

Copy link
Collaborator Author

@TalDerei TalDerei Feb 6, 2025

Choose a reason for hiding this comment

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

thinking about this more, what's the utility of returning SNRs in the response body to the frontend? why not just store them directly in to the LqtVotingNotes IndexedDB table during the rpc call? Our existing logic for querying voting notes in wasm + nullifier rpc should be sufficient to filter eligible voting notes and save them to storage.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we want to make assumptions about shared memory, for example, how would that work if the view server is remote (pclientd) or if we are using the rust view server like in pcli.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that's a good point thanks

@TalDerei TalDerei marked this pull request as draft February 6, 2025 00:45
}

// A list of votes cast for different incentivized assets.
repeated Vote votes = 1;
Copy link
Member

Choose a reason for hiding this comment

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

One thing I forgot to add is the TransactionId for the vote

Copy link
Collaborator Author

@TalDerei TalDerei Feb 6, 2025

Choose a reason for hiding this comment

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

added TransactionId field to TournamentVotesResponse

Copy link
Collaborator Author

@TalDerei TalDerei Feb 6, 2025

Choose a reason for hiding this comment

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

I think we also need the AddressIndex in the request here

@TalDerei TalDerei requested a review from erwanor February 6, 2025 04:32
@TalDerei TalDerei marked this pull request as ready for review February 6, 2025 04:33
@TalDerei TalDerei requested a review from cronokirby February 6, 2025 04:45
@cronokirby cronokirby merged commit ac859ff into protocol/lqt_branch Feb 6, 2025
10 checks passed
@cronokirby cronokirby deleted the prax-view-service-rpcs branch February 6, 2025 18:17
TalDerei added a commit that referenced this pull request Feb 6, 2025
## Describe your changes

appends `AddressIndex` field to `TournamentVotesRequest`

## Issue ticket number and link

references #5062

## 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:

  > LQT branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus-breaking breaking change to execution of on-chain data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants