Skip to content
This repository was archived by the owner on May 21, 2024. It is now read-only.

Conversation

@kalaninja
Copy link
Contributor

@kalaninja kalaninja commented May 12, 2023

  • Fix benchmarks. Benchmarks fail because of this check ensure!(T::Assets::asset_exists(asset_id), Error::<T>::AssetDoesNotExist). The asset being used has to exist in the pallet responsible for storing assets (pallet_assets in our case)
  • Refactor tests. Increased atomicity.

Copy link
Contributor

@hbulgarini hbulgarini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dropped two questions that are not blockers for merging this PR. However i think it would be nice to include a little bit more details in the PR description documenting how this is needed and why the current benchmark process is not working as expected. At least on my side i don't have 100% clear why the helper is needed instead of tweaking the GenesisConfig of the mocked runtime.

@kalaninja
Copy link
Contributor Author

I dropped two questions that are not blockers for merging this PR. However i think it would be nice to include a little bit more details in the PR description documenting how this is needed and why the current benchmark process is not working as expected. At least on my side i don't have 100% clear why the helper is needed instead of tweaking the GenesisConfig of the mocked runtime.

We cannot use mocks in benchmarks. Pallets are implemented abstracting lots of things. Exact implementations of this abstractions affect the real performance. So benchmarks are executed on real runtimes and their results might differ for different runtimes, depending on how you parameterize it. So benchmarks have to be runtime agnostic, because their pallet can be used multiple ways and multiple configurations. Which is a problem in case of pallet coupling.

Since asset-registry is dependent on abstraction over Assets it has to prepare the stage for a benchmark in an abstract way too. BenchmarkHelper is a way for a pallet to say: I don't know how you manage assets in your runtime, but I need you to give me an Id of a registered asset for my benchmarks.

@hbulgarini
Copy link
Contributor

I dropped two questions that are not blockers for merging this PR. However i think it would be nice to include a little bit more details in the PR description documenting how this is needed and why the current benchmark process is not working as expected. At least on my side i don't have 100% clear why the helper is needed instead of tweaking the GenesisConfig of the mocked runtime.

We cannot use mocks in benchmarks. Pallets are implemented abstracting lots of things. Exact implementations of this abstractions affect the real performance. So benchmarks are executed on real runtimes and their results might differ for different runtimes, depending on how you parameterize it. So benchmarks have to be runtime agnostic, because their pallet can be used multiple ways and multiple configurations. Which is a problem in case of pallet coupling.

Since asset-registry is dependent on abstraction over Assets it has to prepare the stage for a benchmark in an abstract way too. BenchmarkHelper is a way for a pallet to say: I don't know how you manage assets in your runtime, but I need you to give me an Id of a registered asset for my benchmarks.

i get it know, i think the implementation looks all right. Thanks for the explanation.

hbulgarini
hbulgarini previously approved these changes May 15, 2023
stiiifff
stiiifff previously approved these changes May 15, 2023
@kalaninja kalaninja dismissed stale reviews from stiiifff and hbulgarini via 3f930c7 May 16, 2023 03:12
@kalaninja kalaninja requested review from hbulgarini and stiiifff May 16, 2023 03:15
@stiiifff stiiifff merged commit 24552f3 into main May 16, 2023
@stiiifff stiiifff deleted the fix/asset-registry-refactor branch May 16, 2023 09:07
@stiiifff stiiifff mentioned this pull request May 16, 2023
18 tasks
stiiifff pushed a commit that referenced this pull request May 25, 2023
Backport PR #173 (cherry picked from commit 24552f3) in Trappist v0.9.40 and updates it to match the changes included in polkadot v0.9.38 to v0.9.40. Weights were re benchmarked to match weightsV2.

Co-authored-by: Valentin Fernandez <[email protected]>
hbulgarini added a commit that referenced this pull request Jul 31, 2023
* Upgrade to v0.9.40 (#171)

* Upgraded to 9-40 missing some fixes

* Added XCM v3 Traits (Need to configure)

* Added fee per MB

* Added XCM V3 Configs

* Fix XCM_config and node service

* Removed chess pallet

* pallet_contracts and pallet_xcm fix

* rust fmt

* Quick fix to contracts on stout and pallet-dex dependancie

* Added hardware check to service

* Fixed stout service

* Review fixes

* run tests on CI (#174)
(cherry picked from commit d826938)

* fmt

* review fix

* review fix

* fmt

* Fixed xcm-playground error

---------

Co-authored-by: Valentin Fernandez <[email protected]>
Co-authored-by: Alexander Kalankhodzhaev <[email protected]>

* Backport Drop Assets (#189)

Backport PR #165 (cherry picked from commit 3f1c483) in Trappist v0.9.40 and updates it to match the changes included in polkadot v0.9.38 to v0.9.40. Weights were re benchmarked to match weightsV2.
---------

Co-authored-by: Valentin Fernandez <[email protected]>

* asset-registry backport (#193)

Backport PR #173 (cherry picked from commit 24552f3) in Trappist v0.9.40 and updates it to match the changes included in polkadot v0.9.38 to v0.9.40. Weights were re benchmarked to match weightsV2.

Co-authored-by: Valentin Fernandez <[email protected]>

* fix CI for substrate version update (#198)

* Backport Zombienet and CI testing (#194)

* zombienet refactor and ci fix

* swap-storage removed from CI

* ci fix

* CI fix

---------

Co-authored-by: Valentin Fernandez <[email protected]>

* Backport Lockdown-pallet  (#191)

* lockdown-pallet backport

* Add lockdown mode activation to lockdown-mode pallet's `GenesisConfig` (#176)

* add activating lockdown mode to GenesisConfig

* add activating lockdown mode to test ext

* add tests for activating lockdown mode in GenesisConfig

* cargo fmt -p pallet-lockdown-mode

* fix linting

* add activated to benchmark

* removing, as this is already in another test

* setting initial state to true, therefore no longer need to manually change state

* rename activated in genesisConfig to initial_status

---------

Co-authored-by: Valentin Fernandez <[email protected]>
Co-authored-by: Bruno Galvao <[email protected]>

* [Backport] #185  XCM benchmarks from `pallet_xcm_benchmarks` to v0.9.40 (#201)

* backport xcm_benchmarks

* Co-Authored-By: hbulgarini <[email protected]>

* fix

* add profile "production" (#187)

---------

Co-authored-by: Alexander Kalankhodzhaev <[email protected]>

* [Backport] #206 Trappist node and #207 workspace refactor to v0.9.40 (#209)

* cherry-picked changes

* WIP fixing node service¨

* Upgrade fixes, missing stout trait bound error

* workspace refactor

* added dex to stout

* fmt

---------

Co-authored-by: Hector Bulgarini <[email protected]>

* Query account balances using `accounts_common` (#202)

* query_account_balances added

* fmt

* Added assets-common dependency from Cumulus

---------

Co-authored-by: Steve Degosserie <[email protected]>

* [Backport] #220 Apache License and #223 Minor Refactoring (#228)

* backport apache license

* backport minor refactoring PR #223

* fmt

* Refactor `WeightToFee` to include ProofSize (#215)

* Removed FixedRateOfFungibles

* fmt

* added comments

* fmt

* removed unused imports

* fmt

* removed allow unpaid execution from rockmine following PR #211

* Include new traders from PR #221

* minor fixes

* minor fixes

* fmt

* [Backport] Asset-registry fix,  Bump Docker and add preimage to benchmarks (#229)

* Validate `MultiLocation` for asset-registry  (#224)

* Validate location for register_reserve_asset

* fmt

* fmt

* fmt

* validator refactor

* fmt

* fix `pallet_xcm_benchmarks::generic::claim_asset` benchmark (#226)

* Bump docker/login-action from 2.1.0 to 2.2.0 (#219)

Bumps [docker/login-action](https://github.com/docker/login-action) from 2.1.0 to 2.2.0.
- [Release notes](https://github.com/docker/login-action/releases)
- [Commits](docker/login-action@f4ef78c...465a078)

---
updated-dependencies:
- dependency-name: docker/login-action
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump docker/metadata-action from 4.4.0 to 4.5.0 (#218)

Bumps [docker/metadata-action](https://github.com/docker/metadata-action) from 4.4.0 to 4.5.0.
- [Release notes](https://github.com/docker/metadata-action/releases)
- [Commits](docker/metadata-action@c4ee3ad...2c0bd77)

---
updated-dependencies:
- dependency-name: docker/metadata-action
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* fix network id in tests

* fix network id in tests

* Add missing import

* include preimage to benchmarks

* Bump docker/metadata-action from 4.5.0 to 4.6.0 (#233)

Bumps [docker/metadata-action](https://github.com/docker/metadata-action) from 4.5.0 to 4.6.0.
- [Release notes](https://github.com/docker/metadata-action/releases)
- [Commits](docker/metadata-action@2c0bd77...818d4b7)

---
updated-dependencies:
- dependency-name: docker/metadata-action
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump docker/build-push-action from 4.0.0 to 4.1.1 (#232)

Bumps [docker/build-push-action](https://github.com/docker/build-push-action) from 4.0.0 to 4.1.1.
- [Release notes](https://github.com/docker/build-push-action/releases)
- [Commits](docker/build-push-action@3b5e802...2eb1c19)

---
updated-dependencies:
- dependency-name: docker/build-push-action
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Alexander Kalankhodzhaev <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* WIP

* WeightToFee updated to use FeePolynomial. Based on the implementation in the asset-hub

* finish upgrade

* update deps

* Addressing check account

* Fix

* fmt

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: valentinfernandez1 <[email protected]>
Co-authored-by: Valentin Fernandez <[email protected]>
Co-authored-by: Alexander Kalankhodzhaev <[email protected]>
Co-authored-by: Bruno Galvao <[email protected]>
Co-authored-by: Steve Degosserie <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Valentin Fernandez <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants