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

[WIP] Add default nonce to frame_system::AccountInfo #4159

Closed
wants to merge 3 commits into from

Conversation

georgepisaltu
Copy link
Contributor

No description provided.

Signed-off-by: georgepisaltu <[email protected]>
@gupnik
Copy link
Contributor

gupnik commented Apr 16, 2024

@georgepisaltu I think #4034 is a better way to do this then.

The one you are exploring was already done here: #1557

Signed-off-by: georgepisaltu <[email protected]>
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5949519

@georgepisaltu
Copy link
Contributor Author

@georgepisaltu I think #4034 is a better way to do this then.

The one you are exploring was already done here: #1557

I now read all of the threads. I just want to provide a working alternative to #4034, which I hope will be simpler. I'd like to avoid the boilerplate code introduced in that newtype solution if possible.

I lightly reviewed #4034 and even if we decide to go with that approach, I'd like to trim as much of that boilerplate as possible.

Signed-off-by: georgepisaltu <[email protected]>
@gupnik
Copy link
Contributor

gupnik commented Apr 16, 2024

@georgepisaltu I think #4034 is a better way to do this then.
The one you are exploring was already done here: #1557

I now read all of the threads. I just want to provide a working alternative to #4034, which I hope will be simpler. I'd like to avoid the boilerplate code introduced in that newtype solution if possible.

I lightly reviewed #4034 and even if we decide to go with that approach, I'd like to trim as much of that boilerplate as possible.

Thanks, but I am not sure if your PR here is different from #1557 ?

Also, could you point me to the boilerplate code that you are referring to? The intent of introducing a new type was to avoid a breaking change if possible.

@georgepisaltu
Copy link
Contributor Author

I am not sure if your PR here is different from #1557 ?

Main difference is that the default is a type on AccountInfo, not on the Account map. I think it's a bit better for testing but looks a bit worse, very small difference.

The intent of introducing a new type was to avoid a breaking change if possible.

I understand. The problem with the newtype is stated here, with a reasonable expectation here.

The point of it was that we could somehow make it passthrough, so we get the most functionality with the least amount of code. The other approach has a new nonce type, NonceWithDefault, could probably be renamed to just Nonce but we'll review in depth later, then we have basic trait impls, non-derived but you can probably get away with deriving the NoBound versions of the traits, then the Deref (needs to be there), Default (main event), then almost 400 lines of boilerplate integer arithmetic, then custom (I mean non-derived) encoding logic, also a blanket impl of a new Nonce trait. I don't think it really is the envisioned solution.

I'm not saying all of this because I think there's something inherently wrong with that PR or the code there, I just want to provide an alternative to that. My reasoning is that, while we're trying to avoid a breaking change, an extra type in frame_system::Config that is nicely abstracted away with all of our default config impls is acceptable. Not only that, but if a user wants to configure their own custom default nonce, I think it's way easier for them to do it with the frame_system::Config solution rather than having them replicate the approach in #4034.

This is just my opinion and this PR is just a draft. If people want to merge #4034 I have no problem with that.

@gupnik
Copy link
Contributor

gupnik commented Apr 16, 2024

The point of it was that we could somehow make it passthrough, so we get the most functionality with the least amount of code. The other approach has a new nonce type, NonceWithDefault, could probably be renamed to just Nonce but we'll review in depth later, then we have basic trait impls, non-derived but you can probably get away with deriving the NoBound versions of the traits, then the Deref (needs to be there), Default (main event), then almost 400 lines of boilerplate integer arithmetic, then custom (I mean non-derived) encoding logic, also a blanket impl of a new Nonce trait. I don't think it really is the envisioned solution.

I understand your concerns, but this type would essentially provided as an additional primitive that could be used directly. So, the runtime dev doesn't need to bother about these complexities at all.

I'm not saying all of this because I think there's something inherently wrong with that PR or the code there, I just want to provide an alternative to that.

Thanks! My intent was not to persuade you to not explore an alternate solution but avoid an implementation if something similar was already tried. But yes, please continue if you think this is better.

@bkchr
Copy link
Member

bkchr commented Apr 18, 2024

The one you are exploring was already done here: #1557

Yeah, I see it the same way. I think we should go with the pr from @gupnik

@georgepisaltu
Copy link
Contributor Author

Closing this in favor of #4034

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.

4 participants