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

lqt: emit events in dex/funding #5065

Merged
merged 21 commits into from
Feb 6, 2025
Merged

Conversation

erwanor
Copy link
Member

@erwanor erwanor commented Feb 6, 2025

Describe your changes

This PR adds events for the LQT tournament

Issue ticket number and link

#5066 #5067

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

@erwanor erwanor changed the title Erwan/lqt events lqt: emit events in dex/funding Feb 6, 2025
@erwanor erwanor marked this pull request as ready for review February 6, 2025 21:19
@erwanor erwanor requested review from cronokirby and TalDerei and removed request for cronokirby February 6, 2025 21:19
// The amount of reward, in delegation tokens.
asset.v1.Value delegation_tokens = 3;
// The recipient of the reward
keys.v1.Address address = 4;
Copy link
Collaborator

@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.

just to clarify, appending the address is fine here because the reward comes from the community pool which mints transparently?

num.v1.Amount position_volume = 11;
}

message EventLqtVote {
Copy link
Collaborator

Choose a reason for hiding this comment

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

what about transaction ID? or does this create a link in the events for the vote, associating it with the transaction hash that performed the voting in the LiquidityTournamentReward?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea

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.

Looks good, I think we could add a couple other bits of information to the events to ambient context. For the vote in particular this is a simple change. I'm less sure about the dex event though.

num.v1.Amount position_volume = 11;
}

message EventLqtVote {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should add the recipient address and the transaction id. Tx IDs would be a good way to track "unique votes" roughly.

// Tracks the LQT eligible volume for a position.
// The volume corresponds to the outflow of staking tokens from
// the position.
message EventLqtPositionVolume {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a good way to link this to the execution of the dex? Like it could be nice to put in a bsod reference or something which would let us know whether or not it ties in to block execution or arb.

Copy link
Member 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 idea, agreed. I think we will have to switch things around. We can do this on a second pass. One way to do this is to emit the event on position_execution. It will take some workshopping to do this without disrupting the method.

@erwanor erwanor merged commit eb52f47 into protocol/lqt_branch Feb 6, 2025
10 checks passed
@erwanor erwanor deleted the erwan/lqt_events branch February 6, 2025 23:07
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.

3 participants