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

refactor!: remove stride's ICA liquid staking modules #186

Merged
merged 2 commits into from
Nov 22, 2024

Conversation

hallazzang
Copy link
Contributor

@hallazzang hallazzang commented Nov 21, 2024

Description

Closes: MILK-151


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@hallazzang hallazzang self-assigned this Nov 21, 2024
@hallazzang hallazzang marked this pull request as ready for review November 22, 2024 05:12
@hallazzang hallazzang requested review from RiccardoM and manu0466 and removed request for RiccardoM November 22, 2024 05:12
Copy link
Contributor

@RiccardoM RiccardoM left a comment

Choose a reason for hiding this comment

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

Everything looks good, however could you please split the changes that are not related to the removal of the modules (the addition of ratelimit and the typo fixes) in another PR please?

This because I would like to keep the commit in the main branch to only deal with the removal, so that if we ever need to revert it we don't affect anything else (e.g. by removing the added key).

@hallazzang
Copy link
Contributor Author

Oh I didn't notice that I accidentally included the typo fix. Also I just realized that I removed the whole ICA functionality which is not this PR meant to. I'll force-push changes again.

@RiccardoM
Copy link
Contributor

@hallazzang Removing the ICA funcionality is fine. We can have another PR for that though 👍

@hallazzang
Copy link
Contributor Author

hallazzang commented Nov 22, 2024

I'll be focusing on removing Stride's code in this PR. Let's remove ICA in another PR. Also, I didn't add ratelimit, I just moved the line above.

@hallazzang hallazzang force-pushed the hallazzang/remove-ica-liquid-staking branch from 4ae4737 to 66c17fd Compare November 22, 2024 06:16
@hallazzang hallazzang requested a review from RiccardoM November 22, 2024 06:20
Copy link
Contributor

@RiccardoM RiccardoM left a comment

Choose a reason for hiding this comment

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

@hallazzang Please take care of creating a new PR to remove the ICA modules. Thanks!

@RiccardoM RiccardoM merged commit 4b8dbb4 into main Nov 22, 2024
17 checks passed
@RiccardoM RiccardoM deleted the hallazzang/remove-ica-liquid-staking branch November 22, 2024 06:27
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.

2 participants