Add base_asset to Market structure and make prediction_markets pallet use it instead of Asset::Ztg #894

vivekvpandya · 2022-12-01T09:00:48Z

Fixes #226

… use it instead of Asset::Ztg

…r not

zrml/prediction-markets/src/orml_asset_registry.rs

…hangelog_for_devs.md

Chralt98

Essentially this is a great work to allow different base assets in prediction markets. I think about unintended consequences....

zrml/prediction-markets/src/orml_asset_registry.rs

zrml/prediction-markets/src/tests.rs

zrml/prediction-markets/src/orml_asset_registry.rs

zrml/market-commons/src/migrations.rs

maltekliemann

Just passing through and leaving a couple of hopefully useful comments. Do you understand what the purpose of using the __private prefix on the imports in the orml_mock_registry module is?

zrml/prediction-markets/src/lib.rs

zrml/prediction-markets/src/tests.rs

zrml/market-commons/src/migrations.rs

primitives/src/market.rs

primitives/src/types.rs

zrml/prediction-markets/src/orml_asset_registry.rs

zrml/prediction-markets/src/mock.rs

Chralt98

Looks okay, when you ask me. I am waiting until the conversations with Malte are resolved.

primitives/src/types.rs

zrml/market-commons/src/migrations.rs

zrml/prediction-markets/src/lib.rs

zrml/prediction-markets/src/tests.rs

Chralt98

Looks pretty good to allow different assets as the denominator in prediction markets.

Chralt98 · 2023-01-17T07:37:09Z

zrml/prediction-markets/Cargo.toml

    "parity-scale-codec/std",
+    "serde/std",


Suggested change

"serde/std",

"serde?/std",

The package serde has also got optional = true. Am I missing something?

zrml/prediction-markets/src/tests.rs

Chralt98 · 2023-01-17T08:23:55Z

Really great work here Vivek! Love that.

maltekliemann

It seems like the CI doesn't run the tests with parachain; it only runs check. I guess you could add
```
cargo test -p zrml-prediction-markets --features parachain
```
to misc.sh (not sure what the exact structure of those scripts is).
One test is still failing.
Is there a test for create_market failing if an invalid base asset is specified?
There are some comments above that are still unaccounted for.

zrml/prediction-markets/src/tests.rs

maltekliemann · 2023-01-17T17:13:54Z

zrml/prediction-markets/src/tests.rs

+        let oracle_bond = <Runtime as Config>::OracleBond::get();
+        // substract min_liquidity twice, one for buy_complete_set() in
+        // create_cpmm_market_and_deploy_assets() and one in swaps::create_pool()
+        // then again substract BASE as buy_complete_set() on line 786


Referring to specific lines in comments is a bad idea.

maltekliemann · 2023-01-17T17:31:31Z

zrml/prediction-markets/src/tests.rs

+            // check native balance
+            let bal = Balances::free_balance(&BOB);
+            assert_eq!(bal, 1_000 * BASE - CENT);
+
+            let market_bal = Balances::free_balance(market_account);
+            assert_eq!(market_bal, CENT);
+        } else {
+            let bal = Tokens::free_balance(base_asset, &BOB);
+            assert_eq!(bal, 1_000 * BASE - CENT);
+
+            let market_bal = Tokens::free_balance(base_asset, &market_account);
+            assert_eq!(market_bal, CENT);


Could have used AssetManager::free_balance and then used base_asset to avoid this if.

vivekvpandya · 2023-01-18T04:00:25Z

Is there a test for create_market failing if an invalid base asset is specified?

please check test create_market_with_foreign_assets() it covers to failure where if a FA is not registered in AssetRegistry and if FA is registered but in metadata its not allowed to be used as base_asset.

codecov-commenter · 2023-01-18T07:10:47Z

Codecov Report

Merging #894 (77b78e4) into main (5a7feaf) will decrease coverage by 0.28%.
The diff coverage is 50.58%.

@@            Coverage Diff             @@
##             main     #894      +/-   ##
==========================================
- Coverage   94.70%   94.42%   -0.28%     
==========================================
  Files          92       93       +1     
  Lines       19368    20258     +890     
==========================================
+ Hits        18342    19129     +787     
- Misses       1026     1129     +103

Flag	Coverage Δ
tests	`94.42% <50.58%> (-0.28%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
primitives/src/types.rs	`3.12% <0.00%> (-30.21%)`	⬇️
zrml/authorized/src/mock.rs	`37.50% <0.00%> (-3.41%)`	⬇️
zrml/court/src/lib.rs	`87.71% <ø> (ø)`
zrml/court/src/mock.rs	`33.33% <0.00%> (-3.51%)`	⬇️
zrml/court/src/tests.rs	`99.20% <ø> (ø)`
zrml/market-commons/src/lib.rs	`93.42% <ø> (ø)`
zrml/market-commons/src/migrations.rs	`0.00% <0.00%> (ø)`
zrml/prediction-markets/src/tests.rs	`99.29% <ø> (-0.60%)`	⬇️
zrml/simple-disputes/src/lib.rs	`77.77% <ø> (ø)`
zrml/simple-disputes/src/mock.rs	`8.69% <0.00%> (-1.31%)`	⬇️
... and 12 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Chralt98

LGTM

scripts/tests/aux-functions.sh

scripts/tests/misc.sh

zrml/prediction-markets/src/tests.rs

maltekliemann

Looks good now.

Just one question (I may have asked this before): Why are we using the parachain feature to begin with? Edit. Seems to me like parachain can be removed from zrml_prediction_markets. Only price to pay is longer compiler times since you need some XCM imports. Seems worth it, though. Am I wrong about this?

@sea212

vivekvpandya · 2023-01-19T03:35:05Z

Looks good now.

Just one question (I may have asked this before): Why are we using the parachain feature to begin with? Edit. Seems to me like parachain can be removed from zrml_prediction_markets. Only price to pay is longer compiler times since you need some XCM imports. Seems worth it, though. Am I wrong about this?

Perhaps it would also bring in some testing time too as there will be XCM tests.
But I think that can be done in separate PR. I think it is better to conclude this PR at this point.

Add base_asset to Market structure and make prediction_markets pallet…

e295166

… use it instead of Asset::Ztg

vivekvpandya added the s:in-progress The pull requests is currently being worked on label Dec 1, 2022

vivekvpandya self-assigned this Dec 1, 2022

vivekvpandya added 5 commits December 5, 2022 15:14

Add AssetRegistry check for foreign asset valid for market creation o…

9edaf3a

…r not

Fix runtime build issues

b75a44a

Warp AssetRegistry related things under parachain feature

08d8e6c

Enable AssetRegistry with parachain feature

a0c3043

Fix build failure

3db2e6a

vivekvpandya commented Dec 5, 2022

View reviewed changes

zrml/prediction-markets/src/orml_asset_registry.rs Show resolved Hide resolved

Add migration for markets to have base_asset. Fix fuzz test. Update c…

c60bc65

…hangelog_for_devs.md

vivekvpandya added s:review-needed The pull request requires reviews and removed s:in-progress The pull requests is currently being worked on labels Dec 6, 2022

vivekvpandya requested review from maltekliemann, sea212 and Chralt98 December 6, 2022 10:13

vivekvpandya marked this pull request as ready for review December 6, 2022 10:14

better log message in migration

3193cac

Chralt98 reviewed Dec 9, 2022

View reviewed changes

maltekliemann added the p:high High priority, prioritize the resolution of this issue label Dec 13, 2022

maltekliemann reviewed Dec 14, 2022

View reviewed changes

vivekvpandya added 4 commits December 16, 2022 14:31

Merge branch 'main' into issue-226

52e5551

Minor changes as per review comments

2fc906b

In Market type add Asset as template parameter instead of MarketId

7fa6c34

Fix build failures and clippy issues

77dcafe

vivekvpandya requested review from maltekliemann and Chralt98 December 18, 2022 06:16

Chralt98 added the i:transactions-changed ⚠️ Implies change in transaction version label Dec 20, 2022

Chralt98 reviewed Dec 20, 2022

View reviewed changes

primitives/src/types.rs Outdated Show resolved Hide resolved

zrml/market-commons/src/migrations.rs Outdated Show resolved Hide resolved

zrml/prediction-markets/src/lib.rs Outdated Show resolved Hide resolved

Changes are per review comments

fa77f63

vivekvpandya requested a review from Chralt98 December 21, 2022 09:27

vivekvpandya requested review from maltekliemann and Chralt98 January 16, 2023 08:26

maltekliemann added this to the v0.3.8 milestone Jan 16, 2023

Chralt98 reviewed Jan 17, 2023

View reviewed changes

zrml/prediction-markets/src/tests.rs Outdated Show resolved Hide resolved

Chralt98 reviewed Jan 17, 2023

View reviewed changes

zrml/prediction-markets/src/tests.rs Outdated Show resolved Hide resolved

Chralt98 reviewed Jan 17, 2023

View reviewed changes

sea212 previously approved these changes Jan 17, 2023

View reviewed changes

sea212 mentioned this pull request Jan 17, 2023

RegistryState in orml_asset_registry.rs will not be required post polkadot v0.9.29 upgrade. #936

Open

maltekliemann requested changes Jan 17, 2023

View reviewed changes

Changes as per reviews

af40a9e

vivekvpandya dismissed sea212’s stale review via af40a9e January 18, 2023 04:57

vivekvpandya requested review from maltekliemann, Chralt98 and sea212 January 18, 2023 04:57

vivekvpandya added 2 commits January 18, 2023 11:47

Merge branch 'main' into issue-226

a1b86ab

Fix tests after merge with main

614bc65

vivekvpandya added 2 commits January 18, 2023 15:32

Fix parachain build for prediction-markets

95abc0c

Fix parachain testing

77b78e4

Chralt98 approved these changes Jan 18, 2023

View reviewed changes

scripts/tests/aux-functions.sh Show resolved Hide resolved

scripts/tests/misc.sh Show resolved Hide resolved

zrml/prediction-markets/src/tests.rs Show resolved Hide resolved

maltekliemann reviewed Jan 18, 2023

View reviewed changes

Chralt98 requested a review from maltekliemann January 19, 2023 06:48

maltekliemann approved these changes Jan 19, 2023

View reviewed changes

vivekvpandya added s:accepted This pull request is ready for merge and removed s:review-needed The pull request requires reviews labels Jan 19, 2023

vivekvpandya merged commit b77b5ac into main Jan 19, 2023

vivekvpandya deleted the issue-226 branch January 19, 2023 10:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add base_asset to Market structure and make prediction_markets pallet use it instead of Asset::Ztg #894

Add base_asset to Market structure and make prediction_markets pallet use it instead of Asset::Ztg #894

vivekvpandya commented Dec 1, 2022 •

edited

Chralt98 left a comment •

edited

maltekliemann left a comment

Chralt98 left a comment •

edited

Chralt98 left a comment

Chralt98 Jan 17, 2023

Chralt98 commented Jan 17, 2023

maltekliemann left a comment

maltekliemann Jan 17, 2023

maltekliemann Jan 17, 2023

vivekvpandya commented Jan 18, 2023

codecov-commenter commented Jan 18, 2023 •

edited

Chralt98 left a comment

maltekliemann left a comment •

edited by Chralt98

vivekvpandya commented Jan 19, 2023

Add base_asset to Market structure and make prediction_markets pallet use it instead of Asset::Ztg #894

Add base_asset to Market structure and make prediction_markets pallet use it instead of Asset::Ztg #894

Conversation

vivekvpandya commented Dec 1, 2022 • edited

Chralt98 left a comment • edited

Choose a reason for hiding this comment

maltekliemann left a comment

Choose a reason for hiding this comment

Chralt98 left a comment • edited

Choose a reason for hiding this comment

Chralt98 left a comment

Choose a reason for hiding this comment

Chralt98 Jan 17, 2023

Choose a reason for hiding this comment

Chralt98 commented Jan 17, 2023

maltekliemann left a comment

Choose a reason for hiding this comment

maltekliemann Jan 17, 2023

Choose a reason for hiding this comment

maltekliemann Jan 17, 2023

Choose a reason for hiding this comment

vivekvpandya commented Jan 18, 2023

codecov-commenter commented Jan 18, 2023 • edited

Codecov Report

Chralt98 left a comment

Choose a reason for hiding this comment

maltekliemann left a comment • edited by Chralt98

Choose a reason for hiding this comment

vivekvpandya commented Jan 19, 2023

vivekvpandya commented Dec 1, 2022 •

edited

Chralt98 left a comment •

edited

Chralt98 left a comment •

edited

codecov-commenter commented Jan 18, 2023 •

edited

maltekliemann left a comment •

edited by Chralt98