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

Moving un-opinionated types and utilities outside of FRAME where possible. #211

Open
JoshOrndorff opened this issue Feb 24, 2023 · 14 comments
Labels
I4-refactor Code needs refactoring. T1-FRAME This PR/Issue is related to core FRAME, the framework.

Comments

@JoshOrndorff
Copy link
Contributor

JoshOrndorff commented Feb 24, 2023

UPDATE: #211


The ensure! macro is useful for checking assumptions in runtime logic. Because most runtimes are currently written in FRAME, this macro has found a home in frame_support.

However, this macro and others are useful in non-frame runtimes as well. I've encountered this issue when helping students write minimal frame-less runtimes at the Polkadot Blockchain Academy, and also while working on the Tuxedo grant.

I propose that we move this macro to somewhere less frame-specific, but I'd like guidance on where that may be. One idea is a new crate called runtime_support. But I'm open to other options as well. Once we agree on where to move it, I'm willing to make the PR.

@yjhmelody
Copy link
Contributor

Maybe sp-support

@bkchr
Copy link
Member

bkchr commented Feb 24, 2023

I'm not convinced. If you need it somewhere else, just copy it. The nice use case of PBA frameless runtime isn't worth moving around stuff that we don't need anywhere else.

@bkchr bkchr closed this as completed Feb 24, 2023
@bkchr
Copy link
Member

bkchr commented Feb 24, 2023

(At some point we will have do to some cleanups of sp-runtime, sp-core and whatever, but even then I would not be sure that ensure! should be moved somewhere else)

@JoshOrndorff
Copy link
Contributor Author

JoshOrndorff commented Feb 24, 2023

PBA frameless runtime isn't worth moving around

Did you see that I mentioned a second usecase? We're working on a utxo-based runtime framework.

Maybe sp-support

If an sp-* crate is the right place, then sp-runtime already exists. But afaik, ensure! is only used in the runtime, not on the client side.

@bkchr
Copy link
Member

bkchr commented Feb 24, 2023

Did you see that I mentioned a second usecase? We're working on a utxo-based runtime framework.

The link didn't worked this morning (was the same docs.rs link). Just copy the macro. I don't see the problem. It is only ~6 lines. This is really nothing that requires an issue or too much discussion.

If an sp-* crate is the right place, then sp-runtime already exists. But afaik, this is only used in the runtime, not on the client side.

sp-runtime is also used in the node at some points, it isn't really separated that greatly right now.

@kianenigma
Copy link
Contributor

kianenigma commented Jul 26, 2023

I'm not convinced. If you need it somewhere else, just copy it. The nice use case of PBA frameless runtime isn't worth moving around stuff that we don't need anywhere else.

I don't fundamentally agree with this. If anything I would say Substrate's ideology is "we do what is semantically correct" to the extent that I know. For example, we have all of these complicated names (eg. instead of calling everything Transaction we have 3 words) for the reason of "we name things such that they best explain what they are", not convenience.

Similarly, I think we should "place things where they are best suited", not where they are most conveniently used.

Either way, I agree that copy pasting works for small things. And it is indeed not a big deal or blocker for anyone. But I'd like to keep a list of things that are not semantically part of FRAME, but are possibly useful outside of it, and as we refactor frame-support and sp-runtime, move them outside.

So far, other than ensure!, I can think of:

  1. with_storage_layer and transactional overlays.

If you don't strongly disagree, let's reopen this, and let it be a tracking issue of items that we think should move to outside of FRAME.

@kianenigma kianenigma reopened this Jul 26, 2023
@kianenigma kianenigma changed the title frame_support::ensure! macro is useful outside of FRAME Moving un-opinionated types and utilities outside of FRAME where possible. Jul 26, 2023
@JoshOrndorff
Copy link
Contributor Author

And the derive macros, CloneNoBound, DebugNoBound, DefaultNoBound.

@bkchr
Copy link
Member

bkchr commented Aug 16, 2023

And the derive macros, CloneNoBound, DebugNoBound, DefaultNoBound.

These macros are good examples of things that could be moved out into their own crate somewhere else. They would probably need some love to make them more flexible, but good in general.

  1. with_storage_layer and transactional overlays.

These are FRAME layers around the bare host functions. While maybe others will use the same way to keep track of layers etc, I can also see other implementations that store the number of layers just in memory. Just because something looks like it could be moved into some general place, doesn't mean it should be done this way.

@juangirini juangirini transferred this issue from paritytech/substrate Aug 24, 2023
@the-right-joyce the-right-joyce added I4-refactor Code needs refactoring. T1-FRAME This PR/Issue is related to core FRAME, the framework. and removed I7-refactor labels Aug 25, 2023
@JoshOrndorff
Copy link
Contributor Author

The most important one I've found so far: the ExecuteBlock trait. https://paritytech.github.io/polkadot-sdk/master/frame_support/traits/trait.ExecuteBlock.html

I want to implement this trait for the Tuxedo Executive, which is analogous to the FRAME executive. Perhaps it should live in sp-runtime?

@bkchr
Copy link
Member

bkchr commented Oct 18, 2023

Just copy it. This is just FRAME internals. You already copied all the macro code for validate_block and this trait was only introduced for this use case.

@JoshOrndorff
Copy link
Contributor Author

JoshOrndorff commented Oct 18, 2023

I'll continue copying stuff for now as I've been doing.

You already copied all the macro code

It's true that I've copied large swaths of your code in my initial attempt to make the Tuxedo runtime work as a parachain. I was thinking of that as the learning and hacking phase though. I was hoping that after I had something working by hacking up your code, there would be some common logic to be extracted into a nice general register_validate_block! macro that could be used for FRAME, Tuxedo, and whatever else comes along in theory or in practice.

My reasoning is that the Substrate APIs themselves (especially, Core, BlockBuilder, TransactionPool) are designed to be flexible enough to accommodate many different runtime paradigms, so perhaps the validate_block implementation could be built from those already-generic APIs alone and not assume anything about frame, tuxedo, etc. The ExecuteBlock trait I mentioned is already conspicuously similar to the Core::execute_block method. Same is true for your CheckInherents trait and the BlockBuilderApi::check_inherents method.

Do you think this is an impossible or impractical goal or bad design direction?

@bkchr
Copy link
Member

bkchr commented Oct 19, 2023

The validate_block implementation generated by Cumulus is soo highly opinionated to FRAME, that it makes no sense to use it somewhere else. Same goes for the register_validate_block macro. I mean you can use this as an inspiration, but we should not try to make it generic, if it doesn't need to. Your implementation for tuxedo will probably look quite different.

@bkchr
Copy link
Member

bkchr commented Oct 19, 2023

I'm totally in favor that you are doing all of this and I think it is a great work. Having a different runtime language. However, just because things are done in FRAME in the way they are done and you also can do the same in tuxedo, doesn't mean that any other implementation would benefit of this.

@muraca
Copy link
Contributor

muraca commented Nov 13, 2023

I started moving the NoBound macros to a new dedicated sp-runtime-procedural crate, and the ensure and fail macros to sp-runtime, see code here.
@bkchr @kianenigma if you like this approach I will open a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I4-refactor Code needs refactoring. T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
Status: Backlog
Development

No branches or pull requests

7 participants