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

remove bech32 ibc #1994

Merged
merged 3 commits into from
Jul 7, 2022
Merged

remove bech32 ibc #1994

merged 3 commits into from
Jul 7, 2022

Conversation

sunnya97
Copy link
Collaborator

@sunnya97 sunnya97 commented Jul 7, 2022

Closes: #XXX

What is the purpose of the change

This PR removes bech32 ibc from the state machine, because it is unmaintained.

Brief Changelog

(for example:)

  • The metadata is stored in the blob store on job creation time as a persistent artifact
  • Deployments RPC transmits only the blob storage reference
  • Daemons retrieve the RPC data from the blob cache

Testing and Verifying

(Please pick one of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added unit test that validates ...
  • Added integration tests for end-to-end deployment with ...
  • Extended integration test for ...
  • Manually verified the change by ...

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? (yes / no)
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (yes / no)
  • How is the feature or change documented? (not applicable / specification (x/<module>/spec/) / Osmosis docs repo / not documented)

@github-actions github-actions bot added the C:app-wiring Changes to the app folder label Jul 7, 2022
@sunnya97
Copy link
Collaborator Author

sunnya97 commented Jul 7, 2022

I can't fully remove bech32 ibc repo as a dependency because its used in the v5 upgrade code. Is it okay to remove old upgrade migrations from newer code releases?

@ValarDragon
Copy link
Member

ValarDragon commented Jul 7, 2022

yeah, just comment out those lines (comment out import as well please)

@sunnya97 sunnya97 marked this pull request as ready for review July 7, 2022 17:39
@sunnya97 sunnya97 requested a review from a team July 7, 2022 17:39
@github-actions github-actions bot added the C:docs Improvements or additions to documentation label Jul 7, 2022
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.

E2E test atm covers sends working AFAIK, so should be confident things are still working. CC'ing @p0mvn to confirm

@ValarDragon ValarDragon requested a review from p0mvn July 7, 2022 18:35
Copy link
Member

@p0mvn p0mvn left a comment

Choose a reason for hiding this comment

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

LGTM - yes as long as e2e tests pass, this should be safe IMO

Sanity checked IBC token transfer e2e test in CI: https://github.com/osmosis-labs/osmosis/runs/7238675443?check_suite_focus=true#step:9:124 . Looks good

@sunnya97 sunnya97 merged commit c79f28b into main Jul 7, 2022
@sunnya97 sunnya97 deleted the remove-bech32ibc branch July 7, 2022 23:57
mattverse pushed a commit that referenced this pull request Jul 20, 2022
* remove bech32 ibc

* comment out in v5 upgrade code

* CHANGELOG
@czarcas7ic czarcas7ic mentioned this pull request Aug 19, 2022
@github-actions github-actions bot mentioned this pull request Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:app-wiring Changes to the app folder C:docs Improvements or additions to documentation
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants