-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add vesting precompile #661
base: main
Are you sure you want to change the base?
Conversation
…faultconfig; first benchmark
… bench fns (both runtime and pallet)
…mpile - Add e2e test to check main vesting functionality - Some format changes - Move endless script line from `package.json` to a separate file (`compile_contracts.sh`) so is more human understandable - Add `extractRevertReason` function so tests are more precise
Add lint check for end-to-end tests in CI workflow
…a-cost Test vesting precompile has a cost
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be more appropiate to name it pallet_vesting_precompile
, something like that. It would be possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about this but in the end I went with pallet_benchmark
. Why? Because the command used to generate the weights targets all the extrinsics:
./target/release/laos \
benchmark \
pallet \
--chain=dev \
--steps=50 \
--repeat=20 \
--pallet=pallet_benchmark \
--extrinsic="*" \
--wasm-execution=compiled \
--output=./runtime/laos/src/weights/
This means that when we add more precompiles' benchmarks, all the weights will go to the same file.
In case we want to separate the weights on a precompile basis, we'd have to run one command per precompile, listing all of the extrinsics of that precompile in the command (e.g. --extrinsic="fn1,fn2,fn3"
...).
The drawback I see here is that we might forget some of the extrinsics when we run the command.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar thing to the other comment, the configuration is about vesting precompile. A renaming here would be needed I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree here. There's only one pallet with one config: benchmark. The pallet and the config include types for all of the required precompiles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file usually serves as a default weights configuration for developers that use the pallet. In this case, benchmark pallet just helps us to run benchmarks for our precompiles. I think we can skip the creation of weights.rs
file here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But such weights are used to register the cost of the call. E.g. in vest
:
<Runtime as crate::Config>::WeightInfo::precompile_vest()
WeightInfo
comes from crate::weights::WeightInfo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, I was trying to think a way to prevent making default weights (i.e.: SubstrateWeight
) public. Because no one needs them. What do you think?
#615
benchmark
(atpallets/benchmark/src
) is required to benchmark precompiles whose code is not included in this repo (i.e. the vesting pallet in this PR)pallets/benchmark/src/precompiles/vesting