Skip to content

split template into 2 opinionated runtimes: AccountId20 + AccountId32#1061

Closed
bernardoaraujor wants to merge 12 commits intomasterfrom
bar/split-template-runtimes
Closed

split template into 2 opinionated runtimes: AccountId20 + AccountId32#1061
bernardoaraujor wants to merge 12 commits intomasterfrom
bar/split-template-runtimes

Conversation

@bernardoaraujor
Copy link
Copy Markdown
Contributor

close #1056

@bernardoaraujor bernardoaraujor requested a review from sorpaas as a code owner May 23, 2023 22:49
bernardoaraujor and others added 5 commits May 24, 2023 10:34
Co-authored-by: Qinxuan Chen <koushiro.cqx@gmail.com>
Co-authored-by: Qinxuan Chen <koushiro.cqx@gmail.com>
@bernardoaraujor bernardoaraujor force-pushed the bar/split-template-runtimes branch from 8dccb25 to f7d72b0 Compare May 25, 2023 22:21
@sorpaas
Copy link
Copy Markdown
Member

sorpaas commented Jun 23, 2023

My problem with this approach is that we now have a lot of duplicate code. You may think we can work with them alright, but it actually is a major source of pain whenever the runtime needs changes (which is frequent).

Would you mind to implement everything as feature gate instead? We continue to have a single runtime but just optionally built with either account types. I saw you already did it like that for some of the types and client construction code.

Also the feature gate accountid32/accountid20 is redundant. You just need to have one feature accountid20. If it's not enabled then we use accountid32.

@bernardoaraujor
Copy link
Copy Markdown
Contributor Author

My problem with this approach is that we now have a lot of duplicate code. You may think we can work with them alright, but it actually is a major source of pain whenever the runtime needs changes (which is frequent).

Would you mind to implement everything as feature gate instead? We continue to have a single runtime but just optionally built with either account types. I saw you already did it like that for some of the types and client construction code.

Also the feature gate accountid32/accountid20 is redundant. You just need to have one feature accountid20. If it's not enabled then we use accountid32.

closing this PR in favor of #1186

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

split template into 2 opinionated runtimes: AccountId20 + AccountId32

3 participants