Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Optional storage hashers in pallet dev mode #13263

Closed
Tracked by #264
kianenigma opened this issue Jan 28, 2023 · 17 comments
Closed
Tracked by #264

Optional storage hashers in pallet dev mode #13263

kianenigma opened this issue Jan 28, 2023 · 17 comments
Assignees
Labels
Z2-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase.

Comments

@kianenigma
Copy link
Contributor

kianenigma commented Jan 28, 2023

In dev-mode, all hashers could default to Blake2_256_Concat.

We could also maintain the same in production, but omit a warning.

An idea that arose in the spirit of simplifying FRAME during the academy.

@kianenigma kianenigma added J2-unconfirmed Issue might be valid, but it’s not yet known. Z2-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. labels Jan 28, 2023
@ruseinov
Copy link
Contributor

It's a good idea to simplify things, although I'm not completely sold on the fact that having an expensive hasher as default is the best idea. For dev mode - maybe. Yes, it is safe. But it also makes users unaware of the price they have to pay and the choices they can make when building pallets.

Maybe we could have some aliases for hashers though, like Safe / Cheap or whatever to make it easier for people to understand and choose between the two? :)

@ggwpez
Copy link
Member

ggwpez commented Feb 13, 2023

But it also makes users unaware of the price they have to pay and the choices they can make when building pallets.

That is basically the point of the dev-mode; to hide complexity from new-comers. Otherwise it is just too complicated to understand. We also allow unbounded storage in there, which you dont want in production as much.

Normally in Rust a map is written as as StorageMap<Key, Value>. If we can get FRAME in dev-mode to be like that, I think it would be easier to understand for new-comers. Probably should also get rid of the annoying map _ prefix, huh?

@ruseinov
Copy link
Contributor

Sounds good, let's do this for dev-mode. However I'm still not sold on hiding this kind of complexity in production, as we work hard to NOT have magic, like for example pallet getters, we should not be easy on the users when they actually have to choose a hashing algorithm suitable for their purpose.

@xlc
Copy link
Contributor

xlc commented Feb 13, 2023

This hide complexity from new-comers and make it harder later for them to build a production pallet. I don't really know if this is the right trade-off.

@gupnik
Copy link
Contributor

gupnik commented Feb 14, 2023

Is it possible to introduce another mode as a "playground" where most things are set to reasonable defaults? I think that it would have definitely helped a lot of folks during the academy.

@kianenigma
Copy link
Contributor Author

However I'm still not sold on hiding this kind of complexity in production, as we work hard to NOT have magic..

The issue explicitly says this is only for dev-mode.

This hide complexity from new-comers and make it harder later for them to build a production pallet. I don't really know if this is the right trade-off.

I get your point, but I am not convinced yet. We want more developers to be capable of onboarding into FRAME and trying new things. dev-mode is meant to enable that. Also, we already hide weight in dev-mode. So your comment is essentially the fact that you don't like dev-mode, not the hasher thing per se.

Is it possible to introduce another mode as a "playground" where most things are set to reasonable defaults? I think that it would have definitely helped a lot of folks during the academy.

I think dev-mode is basically that. It defaults them to something that is enough for you to have a dev/local testnet.

@sam0x17 what is the default weight used in dev-mode?

@sam0x17
Copy link
Contributor

sam0x17 commented Feb 14, 2023

@sam0x17 what is the default weight used in dev-mode?

See here: https://github.com/paritytech/substrate/pull/12536/files#diff-702aefc48c38cab59eddce87758199f5439477ba32125385a924c96ed9505861R220-R222

It is zero. Happy to change this if we think there is a more reasonable number for dev mode. Zero was on the original spec for dev mode that I inherited.

@ruseinov
Copy link
Contributor

ruseinov commented Feb 14, 2023

We could also maintain the same in production, but omit a warning.

I was mostly referring to this part of the issue description which mentions prod setup.

@ruseinov
Copy link
Contributor

So, is this a confirmed issue then or still pending discussion? I've taken a look at what "dev mode" is and I guess I'm convinced that it's fine to hide the hashers. That question will pop up once a pallet get switched over to production mode, which is when we could produce a comprehensive error present a user with a choice. Adoption is important and if this helps that - definitely worth doing it.

@sam0x17
Copy link
Contributor

sam0x17 commented Feb 22, 2023

replicating from my chat with @kianenigma, north star would be something like this:

#[pallet::storage]
type Foo<T:Config> = StorageValue<T::AccountId>;
#[pallet::storage]
type Bar<T> = StorageMap<T::Key, T::AccountId>
#[pallet::storage]
type Bar<T> = StorageDoubleMap<T::Key1, T::Key2, T::AccountId>

@xlc
Copy link
Contributor

xlc commented Feb 22, 2023

@sam0x17 image how you are going to write docs for this. The arguments changes based on some flag elsewhere. I don't think it is a good idea.

@bkchr
Copy link
Member

bkchr commented Feb 22, 2023

Another option would be to just make Blake2_256_Concat the default, always. People could still change this if wanted to whatever they like.

@kianenigma
Copy link
Contributor Author

@sam0x17 image how you are going to write docs for this. The arguments changes based on some flag elsewhere. I don't think it is a good idea.

  • Depending on the error message, this is not bad at all. Imagine the error message is hashers are mandatory except in #[pallet::dev_mode].

  • your comment is overall understandable but you are purely coming from the perspective of a seasoned substrate developer. Imagine how you would approach this as someone new. The problem is, with the existing FRAME having "no simplification by default", you have to explain EVERYTHING at once, and we've strongly seen the pain this causes in PBA. The simplest storage item that you want to teach someone already has some hashers in it and you cannot explain storage without explaining what those hashers mean, which is a downside.

  • Recall that all of this is in the context of dev_mode which is only useful for playing around, while learning, and so on. As I said before:

Also, we already hide weight in dev-mode. So your comment is essentially the fact that you don't like dev-mode, not the hasher thing per se.

@xlc
Copy link
Contributor

xlc commented Feb 23, 2023

Yeah I don't like dev-mode but I don't against as long as it doesn't cause new troubles.

I would image dev mode is a superset of normal mode. i.e. all non-dev mode code should work with dev mode. Otherwise people can't take code from production pallet to dev mode without modify them.

It shouldn't introduce too much new macro magic/syntax as well, which is something I believe we all agree that's not beginner friendly and wanting to reduce them.

So instead of just magically omit a parameter, how about just make it _?

i.e.

#[pallet::storage]
type Bar<T> = StorageMap<_, _, T::Key, T::AccountId>
#[pallet::storage]
type Bar<T> = StorageDoubleMap<_, _, T::Key1, _, T::Key2, T::AccountId>

You don't have to explain exactly what is _ in a hello world tutorial. It is something can be ignored, which align with common usage of _ in other parts of Rust.

@Polkadot-Forum
Copy link

This issue has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/less-magic-in-frame-good-or-bad/2287/1

@kianenigma kianenigma moved this from Backlog to To Do in Runtime / FRAME Mar 10, 2023
@kianenigma
Copy link
Contributor Author

kianenigma commented Apr 1, 2023

I would image dev mode is a superset of normal mode

This is a good north star. If this property is currently held, I am happy to support keeping it that way, ergo _ instead of removing it.

Another avenue would have been leveraging the fact that Rust generics actually support defaults, but then we would have to move the hashers to the end.

This is a bit of an upheaval to the interface, but it would hit both targets at once:

  1. All code without dev_mode works with dev_mode.
  2. All of this _ can be omitted.

That being said, using _ as Default is a common Rust idiom, so not so bad to begin with.

@gupnik gupnik self-assigned this Apr 3, 2023
@gupnik gupnik moved this from To Do to Please Review in Runtime / FRAME Apr 8, 2023
@kianenigma kianenigma removed the J2-unconfirmed Issue might be valid, but it’s not yet known. label Apr 20, 2023
@kianenigma
Copy link
Contributor Author

Closed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Z2-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase.
Projects
Status: Done
Development

No branches or pull requests

8 participants