Runtime: Argo Bridge by freakstatic · Pull Request #5155 · Joystream/joystream

freakstatic · 2024-05-28T11:35:55Z

Still missing the benchmark tests

runtime-modules/argo-bridge/src/Untitled-2

runtime-modules/argo-bridge/src/errors.rs

runtime-modules/argo-bridge/src/events.rs

runtime-modules/argo-bridge/src/errors.rs

runtime-modules/argo-bridge/src/lib.rs

runtime-modules/argo-bridge/src/tests/mod.rs

runtime/src/proposals_configuration/defaults.rs

dobertRowneySr

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

runtime-modules/argo-bridge/src/lib.rs

runtime-modules/argo-bridge/src/types.rs

runtime-modules/argo-bridge/src/tests/mod.rs

dobertRowneySr

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, especially in the happy path assertions (especially on event production)
Also: wouldn't it be easier for you to have multiple commits 😅 , instead of a single huge one ?

dobertRowneySr

Ignore this, I accidentally re requested a review

dobertRowneySr · 2024-05-28T17:34:52Z

bin/node/src/chain_spec/argo_bridge_config.rs

@@ -0,0 +1,21 @@
+use node_runtime::{argo_bridge::types::BridgeStatus, ArgoBridgeConfig};
+
+pub fn production_config() -> ArgoBridgeConfig {


I also think that you can set the mint allowance storage variable to be 0 by default since I don't think any other initial value is plausible

dobertRowneySr

Did another quick review round

runtime/src/lib.rs

dobertRowneySr · 2024-05-28T17:37:40Z

runtime-modules/argo-bridge/src/lib.rs

+        pub PauserAccounts get(fn pauser_accounts): BoundedVec<T::AccountId, T::MaxPauserAccounts>;
+
+        /// Number of tokens that the bridge pallet is able to mint
+        pub MintAllowance get(fn mint_allowance) config(): BalanceOf<T>;


I believe you can set default value of this to 0 if I correctly understood the bridging accounting logic, as there shouldn't be any other possible initial value

in the tests is handy to be able to set a value for the mint allowance in the config

or were you really just suggesting a default value and not removing it from the genesis config?

the storage value mint_allowance is increased during Outbound transfer and decreased during Inbound transfer and it represent the total amount of JOY we have outside of the Joystream mainnet network.
If so my observation was: since the only plausible initial value is zero, we can set the default value when declearing the storage variable inside the decl_storage macro instead of manually setting the value in the chainspec config. (Even though I remember with confidence that in case a storage variable is not initialised it will be set to the fault value when accessed for the first time, so 0 for mint allowance which in production will be a u128)

this is a minor comment however, I am fine even with the existing solution

runtime/src/lib.rs

freakstatic · 2024-05-28T18:22:31Z

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 👍
The operator account being None is because I wasn't sure which account should be the default one 🤔

dobertRowneySr · 2024-05-29T19:28:59Z

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

freakstatic · 2024-05-30T04:36:54Z

@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.")

runtime/src/lib.rs

kdembler · 2024-05-30T09:53:55Z

@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 · 2024-05-30T12:38:33Z

@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 · 2024-05-31T10:47:32Z

@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 · 2024-05-31T14:01:51Z

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

runtime-modules/proposals/codex/src/benchmarking.rs

…s proposal

dobertRowneySr

Benchmarks looks ok in terms of worst case scenario accounting

dobertRowneySr

There are some errors that needs to be fixed (see CI).
You can test the benchmarking code with cargo test -p "pallet-argo-bridge" --features "runtime-benchmarks" (you can also use clippy with the same feature enabled)

kdembler · 2024-06-02T12:10:25Z

@freakstatic @ignazio-bovo

I opened a PR with fixes for clippy issues that we should merge into this PR: freakstatic#2

I have also opened a new PR based on this to add a revert extrinsic: #5158

Please check

fix clippy warnings

kdembler

LGTM pending benchmark&weights approval from @ignazio-bovo

We also need to add overflow protection in some extrinsics but will do in a subsequent PR

dobertRowneySr

LGTM, the mac osx checks will be fixed in my PR here: #5157

kdembler requested review from dobertRowneySr and kdembler May 28, 2024 11:36

kdembler requested changes May 28, 2024

View reviewed changes

WIP: argo bridge

37f3d91

freakstatic force-pushed the feature/argo-bridge branch from abd17d2 to 37f3d91 Compare May 28, 2024 14:35

dobertRowneySr reviewed May 28, 2024

View reviewed changes

dobertRowneySr suggested changes May 28, 2024

View reviewed changes

dobertRowneySr self-requested a review May 28, 2024 17:27

dobertRowneySr reviewed May 28, 2024

View reviewed changes

dobertRowneySr self-requested a review May 28, 2024 17:35

dobertRowneySr suggested changes May 28, 2024

View reviewed changes

freakstatic added 4 commits May 29, 2024 03:13

change Argo Bridge configs

69263df

tests: improve Argo Bridge tests

299ecf8

fix: use the right destination account on finalize_inbound_transfer

5330221

fix: Argo Bridge - add new error (OperatorAccountNotSet)

cbde78c

dobertRowneySr self-requested a review May 29, 2024 19:24

freakstatic added 2 commits May 30, 2024 05:32

add const for number of max remote chains

c649652

Add Argo Bridge benchmark tests

6fe1642

kdembler reviewed May 30, 2024

View reviewed changes

runtime/src/lib.rs Outdated Show resolved Hide resolved

freakstatic added 2 commits May 30, 2024 11:04

change RemoteAccount comment

60830b1

bump spec version to 2004

8f3c830

add Argo Bridge pallet to benchmarks definition

e1db690

tests: add benchmarking tests to Argo Bridge pallet

3c20a1f

update Argo Bridge weights

afedc96

freakstatic commented May 31, 2024

View reviewed changes

runtime-modules/proposals/codex/src/benchmarking.rs Outdated Show resolved Hide resolved

freakstatic added 2 commits May 31, 2024 23:32

Update Proposals Codex benchmarking to include Argo Bridge constraint…

0faf5ec

…s proposal

small improvements on Argo Bridge benchmarking

0fb0b20

freakstatic marked this pull request as ready for review May 31, 2024 23:24

dobertRowneySr approved these changes Jun 2, 2024

View reviewed changes

dobertRowneySr self-requested a review June 2, 2024 10:14

dobertRowneySr suggested changes Jun 2, 2024

View reviewed changes

fix clippy warnings

bed43b0

kdembler mentioned this pull request Jun 2, 2024

add Argo revert_outbound_transfer extrinsic #5158

Merged

1 task

freakstatic and others added 2 commits June 2, 2024 14:26

Merge pull request #2 from kdembler/argo-clippy

0f48abc

fix clippy warnings

fix: add missing argo bridge configs to project codex pallet

b278c08

kdembler approved these changes Jun 3, 2024

View reviewed changes

dobertRowneySr approved these changes Jun 4, 2024

View reviewed changes

kdembler merged commit 9d0f3b7 into Joystream:petra Jun 4, 2024

		@@ -0,0 +1,21 @@
		use node_runtime::{argo_bridge::types::BridgeStatus, ArgoBridgeConfig};

		pub fn production_config() -> ArgoBridgeConfig {

Conversation

freakstatic commented May 28, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dobertRowneySr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dobertRowneySr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dobertRowneySr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dobertRowneySr May 28, 2024

Choose a reason for hiding this comment

Uh oh!

dobertRowneySr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dobertRowneySr May 28, 2024

Choose a reason for hiding this comment

Uh oh!

freakstatic May 29, 2024

Choose a reason for hiding this comment

Uh oh!

freakstatic May 29, 2024

Choose a reason for hiding this comment

Uh oh!

dobertRowneySr May 30, 2024

Choose a reason for hiding this comment

Uh oh!

dobertRowneySr May 30, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

freakstatic commented May 28, 2024

Uh oh!

dobertRowneySr commented May 29, 2024

Uh oh!

freakstatic commented May 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

kdembler commented May 30, 2024

Uh oh!

mnaamani commented May 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

freakstatic commented May 31, 2024

Uh oh!

freakstatic commented May 31, 2024

Uh oh!

Uh oh!

dobertRowneySr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dobertRowneySr left a comment

dobertRowneySr left a comment •

edited

Loading

dobertRowneySr left a comment •

edited

Loading

freakstatic commented May 30, 2024 •

edited

Loading

mnaamani commented May 30, 2024 •

edited

Loading

dobertRowneySr left a comment •

edited

Loading

dobertRowneySr left a comment •

edited

Loading