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

Add hooks for gamm and lockup modules #81

Merged
merged 6 commits into from
Apr 9, 2021
Merged

Add hooks for gamm and lockup modules #81

merged 6 commits into from
Apr 9, 2021

Conversation

sunnya97
Copy link
Collaborator

@sunnya97 sunnya97 commented Apr 8, 2021 •
edited
Loading

Cherrypicked addition of hooks to x/gamm and x/lockup from @Thunnini's work in #80

@sunnya97 sunnya97 changed the title Added hooks for gamm and lockup modules Add hooks for gamm and lockup modules Apr 8, 2021
@sunnya97 sunnya97 added C:x/gamm Changes, features and bugs related to the gamm module. C:x/lockup labels Apr 8, 2021
@sunnya97 sunnya97 force-pushed the gamm-lockup-hooks branch from 17dd544 to fb03b5d Compare April 8, 2021 22:27
ValarDragon
ValarDragon reviewed Apr 8, 2021
View reviewed changes
app/app.go Show resolved Hide resolved
ValarDragon
ValarDragon reviewed Apr 8, 2021
View reviewed changes
app/app.go Show resolved Hide resolved
ValarDragon
ValarDragon approved these changes Apr 8, 2021
View reviewed changes
Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

LGTM

@ValarDragon
Copy link
Member

Update: We're redoing the changes to actually git cherry-pick the history, to get proper accreditation for commit authorship

@ValarDragon ValarDragon self-requested a review April 9, 2021 00:22
@sunnya97 sunnya97 force-pushed the gamm-lockup-hooks branch from 6a3cf3c to 0d4020c Compare April 9, 2021 00:23
@sunnya97
Copy link
Collaborator Author

sunnya97 commented Apr 9, 2021

Fixed 👍

ValarDragon
ValarDragon approved these changes Apr 9, 2021
View reviewed changes
Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

LGTM

@Thunnini
Copy link
Collaborator

Thunnini commented Apr 9, 2021

The reason why the hook was added to the lockup module was because the pool-yield module needed a hook, and it wasn’t implemented at the time. It was mostly a temporary measure. @antstalepresh If you’ve implemented it now, then I think Eugen’s code should be merged. What do you think?

@antstalepresh
Copy link
Contributor

The reason why the hook was added to the lockup module was because the pool-yield module needed a hook, and it wasn’t implemented at the time. It was mostly a temporary measure. @antstalepresh If you’ve implemented it now, then I think Eugen’s code should be merged. What do you think?

I didn't have time to write hooks for lockup module, was focusing on incentives module. But it seems good, already have 2 hooks added to the codebase.

@sunnya97 sunnya97 merged commit 6c07c00 into main Apr 9, 2021
@ValarDragon ValarDragon deleted the gamm-lockup-hooks branch April 9, 2021 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antstalepresh antstalepresh antstalepresh approved these changes

@ValarDragon ValarDragon ValarDragon approved these changes

@Thunnini Thunnini Awaiting requested review from Thunnini

Assignees
No one assigned
Labels
C:x/gamm Changes, features and bugs related to the gamm module. C:x/lockup
Projects
Osmosis Chain Development
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sunnya97 @ValarDragon @Thunnini @antstalepresh