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

added register rebate tx #1138

Merged
merged 12 commits into from
Mar 19, 2024
Merged

added register rebate tx #1138

merged 12 commits into from
Mar 19, 2024

Conversation

sampocs
Copy link
Collaborator

@sampocs sampocs commented Mar 15, 2024

Context

Added transaction to register a rebate by specifying the rebate percentage and liquid stake amount. The transaction stores the relevant parameters on the rebate section of the host zone.

riley-stride
riley-stride approved these changes Mar 16, 2024
View reviewed changes
Copy link
Contributor

@riley-stride riley-stride left a comment

Choose a reason for hiding this comment

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

Looking really good!

Some nit comments, only substantive thing is on the bounds for the inputs to the admin fn - think we should allow rebatePct [0,1] and allow liquidStakedAmount g.t.e. 0

ethan-stride
ethan-stride approved these changes Mar 16, 2024
View reviewed changes
Copy link
Contributor

@ethan-stride ethan-stride left a comment

Choose a reason for hiding this comment

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

LGTM

proto/stride/stakeibc/tx.proto Outdated Show resolved Hide resolved
x/stakeibc/keeper/msg_server.go Outdated Show resolved Hide resolved
@sampocs sampocs requested review from asalzmann and shellvish March 18, 2024 20:20
@riley-stride
Copy link
Contributor

riley-stride commented Mar 18, 2024 •
edited
Loading

Just resolved all my comments, I'm signed off on this PR

shellvish
shellvish approved these changes Mar 19, 2024
View reviewed changes
Copy link
Contributor

@shellvish shellvish left a comment

Choose a reason for hiding this comment

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

Squeaky clean code! A few nit comments, but overall looks very good. I'll check this approach with the dYdX team today and make sure it's all clear with them too.

proto/stride/stakeibc/tx.proto Outdated Show resolved Hide resolved
x/stakeibc/keeper/msg_server.go Outdated Show resolved Hide resolved
x/stakeibc/keeper/msg_server_test.go Outdated Show resolved Hide resolved
x/stakeibc/keeper/msg_server_test.go Outdated Show resolved Hide resolved
asalzmann
asalzmann reviewed Mar 19, 2024
View reviewed changes
Copy link
Contributor

@asalzmann asalzmann left a comment

Choose a reason for hiding this comment

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

lgtm! just one nit around the naming

x/stakeibc/types/codec.go Show resolved Hide resolved
x/stakeibc/client/cli/tx_set_community_pool_rebate.go Show resolved Hide resolved
@sampocs sampocs force-pushed the dydx-rebate/register-rebate branch from 59fde12 to 423a22d Compare March 19, 2024 16:33
@sampocs sampocs force-pushed the dydx-rebate/register-rebate branch from 423a22d to 13c9355 Compare March 19, 2024 16:34
@sampocs sampocs merged commit d0b9578 into rebate Mar 19, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@riley-stride riley-stride riley-stride approved these changes

@shellvish shellvish shellvish approved these changes

@ethan-stride ethan-stride ethan-stride approved these changes

@asalzmann asalzmann asalzmann approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sampocs @riley-stride @asalzmann @shellvish @ethan-stride