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 legacy governance v1beta1 handler for ClientUpdateProposal #5666

Closed
3 tasks done
crodriguezvega opened this issue Jan 19, 2024 · 6 comments · Fixed by #6777
Closed
3 tasks done

Remove legacy governance v1beta1 handler for ClientUpdateProposal #5666

crodriguezvega opened this issue Jan 19, 2024 · 6 comments · Fixed by #6777
Assignees
Labels
deprecated Issues to remove deprecated code good first issue Good for newcomers
Milestone

Comments

@crodriguezvega
Copy link
Contributor

crodriguezvega commented Jan 19, 2024

After completing the migration to v8, it should be safe to remove the handler.

@crodriguezvega crodriguezvega added good first issue Good for newcomers deprecated Issues to remove deprecated code labels Jan 19, 2024
@crodriguezvega crodriguezvega added this to the v9.0.0 milestone Jan 19, 2024
@crodriguezvega
Copy link
Contributor Author

Is it safe to completely remove also (can open a separate issue) the messages for ClientUpdateProposal and UpgradeProposal?

@DimitrisJim
Copy link
Contributor

DimitrisJim commented Jan 22, 2024

Is it safe to completely remove also (can open a separate issue) the messages for ClientUpdateProposal and UpgradeProposal

what would happen if we removed these (and the route in app.go) and we have an inflight prop? Looking at the route in gov, we should panic when calling GetRoute (I recall these being caught but can't find specific code in sdk for it atm).

edit: okay, seems it does a HasRoute before we get it in ExecLegacyContent, should definitely test for that case tho

@crodriguezvega
Copy link
Contributor Author

what would happen if we removed these (and the route in app.go) and we have an inflight prop? Looking at the route in gov, we should panic when calling GetRoute (I recall these being caught but can't find specific code in sdk for it atm).

Yes, this was the issue that could have occurred in the upgrade to v8, but if I remember correctly, we agreed that for the next major upgrade we should remove them. If a chain upgraded to v8 then they should start using the new gov v1 messages and not continue using the deprecated ones.

@DimitrisJim
Copy link
Contributor

DimitrisJim commented Jan 22, 2024

considering I haven't seen folks respecting not using deprecated parts of API 😆 I'd at least wanna make sure that rm-ing them completely couldn't lead to a panic.

@crodriguezvega crodriguezvega moved this to Backlog in ibc-go Jan 25, 2024
@colin-axner
Copy link
Contributor

It should be safe to remove. But if you want to be ultra safe, we can change the ValidateBasic of both proposal types to return an error. This is at least the first step in preventing users from submitting legacy proposal types. Then after some time, we could remove the type and be certain there are no in-flight proposals

@colin-axner
Copy link
Contributor

colin-axner commented Jun 18, 2024

I'm in favour of pure removal. I think the additional defensive step to prevent submission is unnecessary in my opinion. We could backport the validate basic error to v8, but I don't see a reason in keeping the types around in v9 to ensure there are never in-flight proposals

@colin-axner colin-axner moved this from Backlog 🕐 to Todo 🏃 in ibc-go Jun 18, 2024
@DimitrisJim DimitrisJim self-assigned this Jul 8, 2024
@DimitrisJim DimitrisJim moved this from Todo 🏃 to In review 👀 in ibc-go Jul 8, 2024
@github-project-automation github-project-automation bot moved this from In review 👀 to Done 🥳 in ibc-go Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecated Issues to remove deprecated code good first issue Good for newcomers
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants