Take revenue #1228

zjb0807 · 2021-07-19T04:28:12Z

Closes: #1115

shaunxw · 2021-07-19T05:05:00Z

runtime/karura/src/lib.rs

@@ -1060,6 +1062,15 @@ impl OnUnbalanced<NegativeImbalance> for DealWithFees {
 		}
 	}
 }
+impl TakeRevenue for DealWithFees {
+	fn take_revenue(revenue: MultiAsset) {
+		if let MultiAsset::ConcreteFungible { amount, .. } = revenue {


Not all concrete fungible revenue is in KSM, need to check the the location.

shaunxw · 2021-07-19T05:08:43Z

runtime/mandala/src/lib.rs

@@ -1106,6 +1108,15 @@ impl OnUnbalanced<NegativeImbalance> for DealWithFees {
 		}
 	}
 }
+impl TakeRevenue for DealWithFees {
+	fn take_revenue(revenue: MultiAsset) {
+		if let MultiAsset::ConcreteFungible { amount, .. } = revenue {


shaunxw · 2021-07-19T05:08:52Z

runtime/mandala/src/lib.rs

+		if let MultiAsset::ConcreteFungible { amount, .. } = revenue {
+			// ensure KaruraTreasuryAccount have ed for DOT.
+			// Ignore the result.
+			let _ = Tokens::deposit(AUSD, &TreasuryAccount::get(), amount);


Suggested change

let _ = Tokens::deposit(AUSD, &TreasuryAccount::get(), amount);

let _ = Tokens::deposit(DOT, &TreasuryAccount::get(), amount);

shaunxw · 2021-07-19T05:12:26Z

runtime/karura/src/lib.rs

@@ -1060,6 +1062,15 @@ impl OnUnbalanced<NegativeImbalance> for DealWithFees {
 		}
 	}
 }
+impl TakeRevenue for DealWithFees {


I would prefer not to reuse the DealWithFees. We should make each struct type concern their own business. Declare a new struct before XcmConfig and name it ToTreasury.

codecov · 2021-07-19T06:15:39Z

Codecov Report

Merging #1228 (28bd6ca) into master (eb3af16) will decrease coverage by 0.23%.
The diff coverage is 0.00%.

❗ Current head 28bd6ca differs from pull request most recent head 2039536. Consider uploading reports for the commit 2039536 to get more accurate results

@@            Coverage Diff             @@
##           master    #1228      +/-   ##
==========================================
- Coverage   66.04%   65.80%   -0.24%     
==========================================
  Files         224      222       -2     
  Lines       19753    19213     -540     
==========================================
- Hits        13045    12643     -402     
+ Misses       6708     6570     -138

Impacted Files	Coverage Δ
runtime/karura/src/lib.rs	`8.57% <0.00%> (-0.10%)`	⬇️
runtime/mandala/src/lib.rs	`24.26% <0.00%> (-0.25%)`	⬇️
modules/evm/src/runner/handler.rs	`69.86% <0.00%> (-0.27%)`	⬇️
modules/currencies/src/mock.rs
modules/currencies/src/tests.rs
modules/currencies/src/lib.rs
modules/currencies/src/weights.rs
primitives/src/currency.rs
modules/staking-pool/rpc/runtime-api/src/lib.rs
node/src/main.rs	`0.00% <0.00%> (ø)`
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb3af16...2039536. Read the comment docs.

shaunxw · 2021-07-19T06:44:57Z

runtime/karura/src/lib.rs

+impl TakeRevenue for ToTreasury {
+	fn take_revenue(revenue: MultiAsset) {
+		if let MultiAsset::ConcreteFungible { id, amount } = revenue {
+			if CurrencyIdConvert::convert(id).is_some() {


is_some check only ensures id could be converted to a CurrencyId but it doesn't have to be KSM.

To handle it correctly, get the wrapped currency_id in result and deposit with it.

shaunxw

LGTM now. We need to do end to end test before merging it.

zjb0807 · 2021-07-20T10:23:29Z

[
  {
    "phase": {
      "applyExtrinsic": 0
    },
    "event": {
      "index": "0x0000",
      "data": [
        {
          "weight": 185277000,
          "class": "Mandatory",
          "paysFee": "Yes"
        }
      ]
    },
    "topics": []
  },
  {
    "phase": {
      "applyExtrinsic": 1
    },
    "event": {
      "index": "0x1e03",
      "data": [
        1
      ]
    },
    "topics": []
  },
  {
    "phase": {
      "applyExtrinsic": 1
    },
    "event": {
      "index": "0x0003",
      "data": [
        "owxc9EpKLTtSjkRmzSytjmJtBgjVcMdNf5ZwCvR1hDjTGSE"
      ]
    },
    "topics": []
  },
  {
    "phase": {
      "applyExtrinsic": 1
    },
    "event": {
      "index": "0x0b00",
      "data": [
        {
          "token": "KSM"
        },
        "owxc9EpKLTtSjkRmzSytjmJtBgjVcMdNf5ZwCvR1hDjTGSE",
        999952000000
      ]
    },
    "topics": []
  },
  {
    "phase": {
      "applyExtrinsic": 1
    },
    "event": {
      "index": "0x0c02",
      "data": [
        {
          "token": "KSM"
        },
        "owxc9EpKLTtSjkRmzSytjmJtBgjVcMdNf5ZwCvR1hDjTGSE",
        999952000000
      ]
    },
    "topics": []
  },
  {
    "phase": {
      "applyExtrinsic": 1
    },
    "event": {
      "index": "0x0c02",
      "data": [
        {
          "token": "KSM"
        },
        "qmmNufxeWaAVLMER2va1v4w2HbuU683c5gGtuxQG4fKTZSb",
        48000000
      ]
    },
    "topics": []
  },
  {
    "phase": {
      "applyExtrinsic": 1
    },
    "event": {
      "index": "0x3502",
      "data": [
        "0xd0fc0b77005cdc143478c9d30527010bc62d51b8cb1b2c2d37643a3c0d5e14af",
        {
          "complete": 600000000
        }
      ]
    },
    "topics": []
  },
  {
    "phase": {
      "applyExtrinsic": 1
    },
    "event": {
      "index": "0x1e04",
      "data": [
        600000000,
        "0x8ffc8978d9512ac029f7a829ebb1a360066c362c1a7e17bef6212178503b505e"
      ]
    },
    "topics": []
  },
  {
    "phase": {
      "applyExtrinsic": 1
    },
    "event": {
      "index": "0x0000",
      "data": [
        {
          "weight": 0,
          "class": "Mandatory",
          "paysFee": "Yes"
        }
      ]
    },
    "topics": []
  }
]

The treasury(qmmNufxeWaAVLMER2va1v4w2HbuU683c5gGtuxQG4fKTZSb) take revenue of XCM execution fee 48000000.

xlc · 2021-07-20T11:15:18Z

How hard to write some tests for this?

zjb0807 · 2021-07-20T13:46:30Z

How hard to write some tests for this?

BTW, learn about the xcm transaction process.

shaunxw · 2021-07-20T23:11:57Z

We only need to mock the execution of received XCM from Kusama in integration tests. You may refer to the commented out ones in integration tests.

Acala/runtime/mandala/tests/integration_test.rs

Line 1654 in 8c69db0

// fn receive_cross_chain_assets() {

…am-version * origin/master: Take revenue (#1228) Update orml (#1245) Updated the Substrate and Cumulus version to the latest commit on polkadot-v0.9.8 branch (#1242) # Conflicts: # orml

…mprovement * origin/master: Take revenue (#1228) Update orml (#1245) Updated the Substrate and Cumulus version to the latest commit on polkadot-v0.9.8 branch (#1242) # Conflicts: # orml

zjb0807 added 2 commits July 19, 2021 11:20

treasury take revenue xcm fee

3855a8d

treasury take revenue xcm fee

a561745

zjb0807 requested a review from shaunxw July 19, 2021 04:28

shaunxw suggested changes Jul 19, 2021

View reviewed changes

check ConcreteFungible id

80e7783

zjb0807 requested a review from shaunxw July 19, 2021 06:37

shaunxw reviewed Jul 19, 2021

View reviewed changes

zjb0807 added 2 commits July 19, 2021 15:12

Apply review suggestions

71d0709

use Currencies instead of Tokens

28bd6ca

shaunxw approved these changes Jul 19, 2021

View reviewed changes

zjb0807 added 2 commits July 22, 2021 07:46

recover receive_cross_chain_assets

2c689ec

add treasury_should_take_xcm_execution_revenue

2039536

zjb0807 merged commit 9ca456e into master Jul 22, 2021

zjb0807 deleted the take_revenue branch July 22, 2021 03:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Take revenue #1228

Take revenue #1228

zjb0807 commented Jul 19, 2021

shaunxw Jul 19, 2021

shaunxw Jul 19, 2021

shaunxw Jul 19, 2021

shaunxw Jul 19, 2021

codecov bot commented Jul 19, 2021 •

edited

Loading

shaunxw Jul 19, 2021

shaunxw left a comment

zjb0807 commented Jul 20, 2021

xlc commented Jul 20, 2021

zjb0807 commented Jul 20, 2021

shaunxw commented Jul 20, 2021

	let _ = Tokens::deposit(AUSD, &TreasuryAccount::get(), amount);
	let _ = Tokens::deposit(DOT, &TreasuryAccount::get(), amount);

Take revenue #1228

Take revenue #1228

Conversation

zjb0807 commented Jul 19, 2021

shaunxw Jul 19, 2021

Choose a reason for hiding this comment

shaunxw Jul 19, 2021

Choose a reason for hiding this comment

shaunxw Jul 19, 2021

Choose a reason for hiding this comment

shaunxw Jul 19, 2021

Choose a reason for hiding this comment

codecov bot commented Jul 19, 2021 • edited Loading

Codecov Report

shaunxw Jul 19, 2021

Choose a reason for hiding this comment

shaunxw left a comment

Choose a reason for hiding this comment

zjb0807 commented Jul 20, 2021

xlc commented Jul 20, 2021

zjb0807 commented Jul 20, 2021

shaunxw commented Jul 20, 2021

codecov bot commented Jul 19, 2021 •

edited

Loading