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

custom weight function wrapper #4158

Merged
merged 12 commits into from
Jan 14, 2020
Merged

custom weight function wrapper #4158

merged 12 commits into from
Jan 14, 2020

Conversation

kianenigma
Copy link
Contributor

Allows easily declaring weights dynamically based on the function argument types. Can be reviewed but need to wait for #4124 to have proper testing first.

cc @JoshOrndorff

@kianenigma kianenigma added the A0-please_review Pull request needs code review. label Nov 20, 2019
@JoshOrndorff
Copy link
Contributor

I like the idea of writing the weighting function right next to the actual dispatchable call. I'm not a fan of the mapping from integers to DispatchClasses because:

  1. It is not obvious when I see the weighting function, what that second integer actually means.
  2. It provides an opportunity for code to panic (by providing a u8 that doesn't map to a DispatchClass).

I think if you want syntactic sugar to make this easier for the programmer, then allow them to omit the dispatch class entirely and have it default to Normal. IMO the integers are just one more thing that an author would need to consult the docs for.

paint/example/src/lib.rs Outdated Show resolved Hide resolved
@@ -482,6 +482,15 @@ decl_module! {
Ok(())
}

// TODO: this stuff will move to a new test.
#[weight = sr_primitives::weights::FunctionOf(
|args: (&u32, &T::AccountId, &BalanceOf<T>)| (args.0 + 100) as Weight,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to have type inferring to avoid unnecessary code?

Suggested change
|args: (&u32, &T::AccountId, &BalanceOf<T>)| (args.0 + 100) as Weight,
|(val, _, _)| val + 100,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sadly, as far s I tried, no

@kianenigma kianenigma requested a review from gui1117 as a code owner November 22, 2019 14:33
@kianenigma
Copy link
Contributor Author

I might not be able to follow a lot here. This can be either merged or left stale for the time being. It will be needed for the next gen. of weights and the new struct is not yet used. Merging it is also harmless and it is done on my opinion. Just have to make sure that there are no merge conflicts, specifically with #4165

@bkchr bkchr added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Dec 8, 2019
@kianenigma kianenigma added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Dec 20, 2019
@kianenigma
Copy link
Contributor Author

@gavofyork you can merge this already if you need it, but I am working on closures around FunctionOf to further ease the use of it.

@kianenigma kianenigma mentioned this pull request Dec 28, 2019
3 tasks
frame/support/src/weights.rs Outdated Show resolved Hide resolved

impl<Args, F, Class> ClassifyDispatch<Args> for FunctionOf<F, Class>
where
Class: Into<DispatchClass> + Clone,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not entirely sure about the interest of having the Class abstract here ? can't we just have DispatchClass as second field of FunctionOf directly ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I will change it to that. The purpose was to provide a syntactic simplicity. I will do that later with a macro.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed.

@gavofyork
Copy link
Member

how are we doing here @kianenigma ?

@kianenigma
Copy link
Contributor Author

@gavofyork I will address @thiolliere 's comments soon and label the PR for you merge it in. But don't use it everywhere just yet; maybe just in occasions that we really need a function-like weight. I have some pending changes to
1- further ease the use of FuncitonOf() via macro
2- deprecate SimpleDispatchInfo.

Those will come in another PR

@kianenigma kianenigma added A8-looksgood and removed A0-please_review Pull request needs code review. labels Jan 10, 2020
frame/support/src/weights.rs Outdated Show resolved Hide resolved
Co-Authored-By: thiolliere <gui.thiolliere@gmail.com>
@gavofyork
Copy link
Member

@kianenigma tests failing...

@kianenigma kianenigma merged commit c93a0a3 into master Jan 14, 2020
@kianenigma kianenigma deleted the kiz-fn-weight branch January 14, 2020 07:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants