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

fix: replace stakingplus module with staking hooks #1271

Merged
merged 12 commits into from
Mar 9, 2024
Merged

fix: replace stakingplus module with staking hooks #1271

merged 12 commits into from
Mar 9, 2024

Conversation

ulbqb
Copy link
Member

@ulbqb ulbqb commented Mar 7, 2024 •
edited
Loading

Description

This PR replaces stakingplus module with staking hooks. It makes redundant files removed and maintenance easy.

This change has breaking change of proto.

Motivation and context

How has this been tested?

Screenshots (if appropriate):

Checklist:

  • I followed the contributing guidelines and code of conduct.
  • I have added a relevant changelog to CHANGELOG.md
  • I have added tests to cover my changes.
  • I have updated the documentation accordingly.
  • I have updated API documentation client/docs/swagger-ui/swagger.yaml

@ulbqb ulbqb self-assigned this Mar 7, 2024
@ulbqb ulbqb force-pushed the fix/use_stakinghook branch from 591d68c to 14631f6 Compare March 7, 2024 04:41
@ulbqb ulbqb marked this pull request as ready for review March 7, 2024 05:17
@ulbqb ulbqb requested a review from a team as a code owner March 7, 2024 05:17
@ulbqb
Copy link
Member Author

ulbqb commented Mar 7, 2024

I have an issue.
Should stakingplus proto file be modified? This PR makes modification to be minimal files. It's also possible to use proto file without modification. But if so, it contains redundant files.

@tkxkd0159
Copy link
Member

tkxkd0159 commented Mar 7, 2024 •
edited
Loading

I have an issue. Should stakingplus proto file be modified? This PR makes modification to be minimal files. It's also possible to use proto file without modification. But if so, it contains redundant files.

I think current status looks not bad. And please add description about this PR(reason of deprecating stakingplus and new feature(hook) plus CHANGELOG

@ulbqb ulbqb changed the title fix: use staking hook fix: remove stakingplus module by staking hooks Mar 7, 2024
@ulbqb ulbqb changed the title fix: remove stakingplus module by staking hooks fix: replace stakingplus module with staking hooks Mar 7, 2024
@ulbqb ulbqb mentioned this pull request Mar 7, 2024
Closed
5 tasks
tkxkd0159
tkxkd0159 previously approved these changes Mar 7, 2024
View reviewed changes
@ulbqb ulbqb requested review from 0Tech and jaeseung-bae March 7, 2024 09:05
jaeseung-bae
jaeseung-bae reviewed Mar 7, 2024
View reviewed changes
Copy link
Contributor

@jaeseung-bae jaeseung-bae left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Is there any test for hooks to work fine?
I think it's also fine if there is any test in upstream.

x/foundation/keeper/exported.go Outdated Show resolved Hide resolved
x/foundation/keeper/internal/hooks.go Outdated Show resolved Hide resolved
@ulbqb ulbqb merged commit 9db31c4 into Finschia:bumpup50 Mar 9, 2024
32 of 33 checks passed
@ulbqb ulbqb deleted the fix/use_stakinghook branch March 9, 2024 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaeseung-bae jaeseung-bae jaeseung-bae approved these changes

@0Tech 0Tech 0Tech approved these changes

@tkxkd0159 tkxkd0159 Awaiting requested review from tkxkd0159

Assignees

@ulbqb ulbqb

Labels
A: build A: CI C:x/foundation x/foundation module C:x/stakingplus
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ulbqb @tkxkd0159 @0Tech @jaeseung-bae