Joystream / joystream

Joystream Monorepo
http://www.joystream.org
GNU General Public License v3.0
1.42k stars 115 forks source link

Runtime: Argo Bridge #5155

Closed freakstatic closed 3 months ago

freakstatic commented 3 months ago

Still missing the benchmark tests

freakstatic commented 3 months ago

The pallet looks solid, the only issues that I have found are:

  • remote chain address formatting : 12 most significant bytes should be zero IMHO
  • If you can provide a case where the operator account is None The tests needs some touching

Thanks for the review :+1: The operator account being None is because I wasn't sure which account should be the default one :thinking:

ignazio-bovo commented 3 months ago

All my previous comments have been addressed. So for now LGTM Should I wait for the benchmarks?

freakstatic commented 3 months ago

@ignazio-bovo and @kdembler I have added the benchmark tests please take a look. the weight calculation is not finding the benchmarks so I'm most likely missing something...

./scripts/../target/debug/joystream-node benchmark pallet --pallet=argo_bridge --extrinsic="*" --chain=prod-test --steps=50 --repeat=20 --execution=wasm --template=./scripts/../devops/joystream-pallet-weight-template.hbs --output=./scripts/../runtime-modules/argo-bridge/src/weights.rs

2024-05-30 05:28:07 [0] 💸 generated 1 npos voters, 1 from validators and 0 nominators    
2024-05-30 05:28:07 [0] 💸 generated 1 npos targets    
Error: Input("No benchmarks found which match your input.")
kdembler commented 3 months ago

@ignazio-bovo @freakstatic I think the discussion on storage initialization is still not resolved. @ignazio-bovo could you expand on what you had in mind?

mnaamani commented 3 months ago

@ignazio-bovo and @kdembler I have added the benchmark tests please take a look. the weight calculation is not finding the benchmarks so I'm most likely missing something...

./scripts/../target/debug/joystream-node benchmark pallet --pallet=argo_bridge --extrinsic="*" --chain=prod-test --steps=50 --repeat=20 --execution=wasm --template=./scripts/../devops/joystream-pallet-weight-template.hbs --output=./scripts/../runtime-modules/argo-bridge/src/weights.rs

2024-05-30 05:28:07 [0] 💸 generated 1 npos voters, 1 from validators and 0 nominators    
2024-05-30 05:28:07 [0] 💸 generated 1 npos targets    
Error: Input("No benchmarks found which match your input.")

I think you need to add the argo bridge pallet to the define_benchmarks! macro in runtime/src/runtime_api.rs

diff --git a/runtime/src/runtime_api.rs b/runtime/src/runtime_api.rs
index 8d3a06316a..c57e968e51 100644
--- a/runtime/src/runtime_api.rs
+++ b/runtime/src/runtime_api.rs
@@ -127,6 +127,7 @@ mod benches {
         [content, Content]
         [project_token, ProjectToken]
         [pallet_proxy, Proxy]
+        [argo_bridge, ArgoBridge]
     );
 }
freakstatic commented 3 months ago

@ignazio-bovo and @kdembler I have added the benchmark tests please take a look. the weight calculation is not finding the benchmarks so I'm most likely missing something... ./scripts/../target/debug/joystream-node benchmark pallet --pallet=argo_bridge --extrinsic="*" --chain=prod-test --steps=50 --repeat=20 --execution=wasm --template=./scripts/../devops/joystream-pallet-weight-template.hbs --output=./scripts/../runtime-modules/argo-bridge/src/weights.rs

2024-05-30 05:28:07 [0] 💸 generated 1 npos voters, 1 from validators and 0 nominators    
2024-05-30 05:28:07 [0] 💸 generated 1 npos targets    
Error: Input("No benchmarks found which match your input.")

I think you need to add the argo bridge pallet to the define_benchmarks! macro in runtime/src/runtime_api.rs

diff --git a/runtime/src/runtime_api.rs b/runtime/src/runtime_api.rs
index 8d3a06316a..c57e968e51 100644
--- a/runtime/src/runtime_api.rs
+++ b/runtime/src/runtime_api.rs
@@ -127,6 +127,7 @@ mod benches {
         [content, Content]
         [project_token, ProjectToken]
         [pallet_proxy, Proxy]
+        [argo_bridge, ArgoBridge]
     );
 }

thanks very much 🙏 yeah that was exactly what I was missing

freakstatic commented 3 months ago

@ignazio-bovo and @kdembler can you please review the benchmarking tests?

kdembler commented 3 months ago

@freakstatic @ignazio-bovo

I opened a PR with fixes for clippy issues that we should merge into this PR: https://github.com/freakstatic/joystream/pull/2

I have also opened a new PR based on this to add a revert extrinsic: https://github.com/Joystream/joystream/pull/5158

Please check