AstarNetwork / astar-frame

Core frame modules for Astar & Shiden network.
Other
58 stars 38 forks source link

Feature/dapps staking root only registration #69

Closed shunsukew closed 2 years ago

shunsukew commented 2 years ago

Pull Request Summary Dapps staking root-only contract registration.

Right now, when devs register a dapp for dapps-staking, they first need to be whitelisted. But only their account is whitelisted, we don’t know exactly which dapp they will deploy. If we make the entire process atomic without developer_pre_approval process, whoever wants to register can specify exactly what the dev account is and which contract will be used. Also, this is required for governance over dapps registration, because RootOrigin call will be scheduled to pallet-scheduler by pallet-democracy once referendums passed.

Check list

Logs of try-runtime against Shibuya

Screen Shot 2022-07-10 at 1 28 20
shunsukew commented 2 years ago

next pallet version could be V3_1_0, not V4_0_0

Dinonard commented 2 years ago

One thing to also consider - the weights.rs are no longer valid since you've changed the interface. That should be updated.

We should re-run the benchmarks but we don't have a dedicated benchmarking machine. I'd suggest you just remove the weight consumed by pre-approval checks that have now been removed.

shunsukew commented 2 years ago

One thing to also consider - the weights.rs are no longer valid since you've changed the interface. That should be updated.

We should re-run the benchmarks but we don't have a dedicated benchmarking machine. I'd suggest you just remove the weight consumed by pre-approval checks that have now been removed.

I know my local machine is weaker than dedicated benchmarking machine satisfying requirements. but local execution had the same result this time.

========
Storage: DappsStaking PalletDisabled (r:1 w:0)
Storage: DappsStaking RegisteredDevelopers (r:1 w:1)
Storage: DappsStaking RegisteredDapps (r:1 w:1)

How did you define base weight 32_139_000?

Dinonard commented 2 years ago

One thing to also consider - the weights.rs are no longer valid since you've changed the interface. That should be updated. We should re-run the benchmarks but we don't have a dedicated benchmarking machine. I'd suggest you just remove the weight consumed by pre-approval checks that have now been removed.

I know my local machine is weaker than dedicated benchmarking machine satisfying requirements. but local execution had the same result this time.

========
Storage: DappsStaking PalletDisabled (r:1 w:0)
Storage: DappsStaking RegisteredDevelopers (r:1 w:1)
Storage: DappsStaking RegisteredDapps (r:1 w:1)

How did you define base weight 32_139_000?

I didn't define it, it was measured by benchmarking tool.

The part you pasted would be same on any machine, it's discrete measurement of number of DB operations.

It's strange that only one read is removed since the two reads for PreApproval have been removed. But I see now that benchmarking was updated to take into consideration both branches - one with pre-approval enabled, one without. Anyhow, the modification you did is fine. :+1:

shunsukew commented 2 years ago

One thing to also consider - the weights.rs are no longer valid since you've changed the interface. That should be updated. We should re-run the benchmarks but we don't have a dedicated benchmarking machine. I'd suggest you just remove the weight consumed by pre-approval checks that have now been removed.

I know my local machine is weaker than dedicated benchmarking machine satisfying requirements. but local execution had the same result this time.

========
Storage: DappsStaking PalletDisabled (r:1 w:0)
Storage: DappsStaking RegisteredDevelopers (r:1 w:1)
Storage: DappsStaking RegisteredDapps (r:1 w:1)

How did you define base weight 32_139_000?

I didn't define it, it was measured by benchmarking tool.

The part you pasted would be same on any machine, it's discrete measurement of number of DB operations.

It's strange that only one read is removed since the two reads for PreApproval have been removed. But I see now that benchmarking was updated to take into consideration both branches - one with pre-approval enabled, one without. Anyhow, the modification you did is fine. πŸ‘

I see. Oh, I wonder why I thought so... Yes this will calculate actual weight it costs.

T::DbWeight::get().reads(3 as Weight)
shunsukew commented 2 years ago

next pallet version could be V3_1_0, not V4_0_0

We broke register API backward compatibility, so v4_0_0 should be fine.

github-actions[bot] commented 2 years ago

Code Coverage

Package Line Rate Branch Rate Health
precompiles/utils/src 84% 0% βœ”
chain-extensions/types/dapps-staking/src 0% 0% ❌
precompiles/dapps-staking/src 95% 0% βœ”
frame/dapps-staking/src/pallet 85% 0% βœ”
precompiles/substrate-ecdsa/src 78% 0% βž–
precompiles/utils/macro/tests 100% 0% βœ”
frame/collator-selection/src 89% 0% βœ”
frame/dapps-staking/src 93% 0% βœ”
precompiles/utils/src/data 72% 0% βž–
frame/custom-signatures/src 80% 0% βœ”
chain-extensions/trait/src 0% 0% ❌
primitives/xcm/src 88% 0% βœ”
precompiles/assets-erc20/src 91% 0% βœ”
chain-extensions/impls/dapps-staking/src 0% 0% ❌
frame/block-reward/src 96% 0% βœ”
frame/pallet-xcm/src 80% 0% βž–
frame/vesting/src 87% 0% βœ”
precompiles/sr25519/src 78% 0% βž–
precompiles/utils/macro/src 0% 0% ❌
frame/xc-asset-config/src 88% 0% βœ”
precompiles/xcm/src 80% 0% βž–
Summary 85% (8317 / 9801) 0% (0 / 0) βœ”

Minimum allowed line rate is 60%