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

Fix Asset Hub naming in code and release notes #71

Conversation

liamaharon
Copy link
Contributor

@liamaharon liamaharon commented Oct 22, 2023

Cleans up some usage of old naming conventions.

I noticed also in the last release the old naming conventions are used. I believe we should refer to these chains as Asset Hub now?

Copy link
Contributor

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

The names are based on the spec_name found in the runtimes. We should rename the spec names.

@joepetrowski
Copy link
Contributor

The names are based on the spec_name found in the runtimes. We should rename the spec names.

Unfortunately a lot of wallets depend on this. I left a note about it in the runtime. @TarikGul how much would break if we changed this now?

@bkchr
Copy link
Contributor

bkchr commented Oct 22, 2023

Then we need to adapt our release scripts.

@TarikGul
Copy link

The names are based on the spec_name found in the runtimes. We should rename the spec names.

Unfortunately a lot of wallets depend on this. I left a note about it in the runtime. @TarikGul how much would break if we changed this now?

I know many outside tooling is aware of the change and have been preparing. Even in our tooling we made sure to give support for whats to become legacy, and the new spec_names.

how much would break if we changed this now?

Within our tools (under integrations), nothing. Externally, it's hard to tell but I always expect a decent amount of users to be reactive as opposed to proactive, so I always expect some amount of breaking change for people that will require support.

My thoughts:

  1. it "should" be a trivial change for users,
  2. This change has been known for a while so I would hope people have been preparing.
  3. If its in some type of release notes as a breaking change than its a fair change.

liamaharon and others added 2 commits October 23, 2023 09:58
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
@liamaharon
Copy link
Contributor Author

@TarikGul @bkchr so consensus is to make that breaking spec_name change in this PR?

@joepetrowski
Copy link
Contributor

@TarikGul @bkchr so consensus is to make that breaking spec_name change in this PR?

I'm OK with it.

@bkchr
Copy link
Contributor

bkchr commented Oct 23, 2023

If that breaks downstream users, we can also just fix up the release scripts. I don't really care and I also don't really know if someone is using this.

@ggwpez
Copy link
Member

ggwpez commented Oct 23, 2023

If that breaks downstream users, we can also just fix up the release scripts

IMHO this is better instead of just breaking stuff again.

Otherwise the only thing that comes to my mind is that the upgrades would have to applied with force through System::set_code_without_checks.

@liamaharon liamaharon changed the title Clean up usage of old Asset Hub naming conventions Clean up usage of deprecated Asset Hub naming Oct 24, 2023
@liamaharon liamaharon changed the title Clean up usage of deprecated Asset Hub naming Fix Asset Hub naming in code and release notes Oct 24, 2023
@liamaharon
Copy link
Contributor Author

liamaharon commented Oct 24, 2023

In light of the suggestions to not make breaking changes, I have opted to not update the spec names but instead modify the release script to substitute "Statemint" and "Statemine" for "Asset Hub Polkadot (previously Statemint)" and "Asset Hub Kusama (previously Statemine)".

Tested this on my fork: https://github.com/liamaharon/runtimes/releases/tag/v1.0.0

Copy link
Contributor

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Ty!

@liamaharon liamaharon mentioned this pull request Nov 28, 2023
5 tasks
@ordian
Copy link
Contributor

ordian commented Dec 1, 2023

/merge

@fellowship-merge-bot fellowship-merge-bot bot merged commit 16e5564 into polkadot-fellows:main Dec 1, 2023
16 checks passed
@fellowship-merge-bot
Copy link
Contributor

Enabled auto-merge in Pull Request

Available commands
  • /merge: Enables auto-merge for Pull Request
  • /merge cancel: Cancels auto-merge for Pull Request
  • /merge help: Shows this menu

For more information see the documentation

@liamaharon liamaharon deleted the liam-rename-statemint-statemine branch December 1, 2023 12:43
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.

9 participants