Runtime: Argo Bridge #5155

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

ignazio-bovo

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

ignazio-bovo

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 ?

ignazio-bovo

Ignore this, I accidentally re requested a review

ignazio-bovo · 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

ignazio-bovo

Did another quick review round

runtime/src/lib.rs

ignazio-bovo · 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 🤔

ignazio-bovo · 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

ignazio-bovo

Benchmarks looks ok in terms of worst case scenario accounting

ignazio-bovo

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

ignazio-bovo

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

kdembler requested review from ignazio-bovo 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

ignazio-bovo reviewed May 28, 2024

View reviewed changes

ignazio-bovo suggested changes May 28, 2024

View reviewed changes

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

ignazio-bovo reviewed May 28, 2024

View reviewed changes

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

ignazio-bovo 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

ignazio-bovo 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

ignazio-bovo approved these changes Jun 2, 2024

View reviewed changes

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

ignazio-bovo 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

ignazio-bovo approved these changes Jun 4, 2024

View reviewed changes

kdembler merged commit 9d0f3b7 into Joystream:petra Jun 4, 2024
12 of 24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Runtime: Argo Bridge #5155

Runtime: Argo Bridge #5155

freakstatic commented May 28, 2024

ignazio-bovo left a comment

ignazio-bovo left a comment •

edited

Loading

ignazio-bovo left a comment •

edited

Loading

ignazio-bovo May 28, 2024

ignazio-bovo left a comment

ignazio-bovo May 28, 2024

freakstatic May 29, 2024

freakstatic May 29, 2024

ignazio-bovo May 30, 2024

ignazio-bovo May 30, 2024

freakstatic commented May 28, 2024

ignazio-bovo commented May 29, 2024

freakstatic commented May 30, 2024 •

edited

Loading

kdembler commented May 30, 2024

mnaamani commented May 30, 2024 •

edited

Loading

freakstatic commented May 31, 2024

freakstatic commented May 31, 2024

ignazio-bovo left a comment •

edited

Loading

ignazio-bovo left a comment

kdembler commented Jun 2, 2024

kdembler left a comment

ignazio-bovo left a comment •

edited

Loading

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

		pub fn production_config() -> ArgoBridgeConfig {

Runtime: Argo Bridge #5155

Runtime: Argo Bridge #5155

Conversation

freakstatic commented May 28, 2024

ignazio-bovo left a comment

Choose a reason for hiding this comment

ignazio-bovo left a comment • edited Loading

Choose a reason for hiding this comment

ignazio-bovo left a comment • edited Loading

Choose a reason for hiding this comment

ignazio-bovo May 28, 2024

Choose a reason for hiding this comment

ignazio-bovo left a comment

Choose a reason for hiding this comment

ignazio-bovo May 28, 2024

Choose a reason for hiding this comment

freakstatic May 29, 2024

Choose a reason for hiding this comment

freakstatic May 29, 2024

Choose a reason for hiding this comment

ignazio-bovo May 30, 2024

Choose a reason for hiding this comment

ignazio-bovo May 30, 2024

Choose a reason for hiding this comment

freakstatic commented May 28, 2024

ignazio-bovo commented May 29, 2024

freakstatic commented May 30, 2024 • edited Loading

kdembler commented May 30, 2024

mnaamani commented May 30, 2024 • edited Loading

freakstatic commented May 31, 2024

freakstatic commented May 31, 2024

ignazio-bovo left a comment • edited Loading

Choose a reason for hiding this comment

ignazio-bovo left a comment

Choose a reason for hiding this comment

kdembler commented Jun 2, 2024

kdembler left a comment

Choose a reason for hiding this comment

ignazio-bovo left a comment • edited Loading

Choose a reason for hiding this comment

ignazio-bovo left a comment •

edited

Loading

ignazio-bovo left a comment •

edited

Loading

freakstatic commented May 30, 2024 •

edited

Loading

mnaamani commented May 30, 2024 •

edited

Loading

ignazio-bovo left a comment •

edited

Loading

ignazio-bovo left a comment •

edited

Loading