Fix xcmp_fee for automation-time #366

imstar15 · 2023-06-09T16:39:54Z

No description provided.

chrisli30

@imstar15 can you add comments on this webpage pointing out the core changes of this PR?

What is the TransactInfo before change? What are the keys and values?
What is the TransactInfo after? What are the keys and values?
If any extrinsic interface has changed, please provide screenshots for before and after.

Adding @v9n for a look.

chrisli30 · 2023-07-01T00:54:24Z

In addition, "This branch is out-of-date with the base branch". Please rebase master.

chrisli30 · 2023-07-01T01:26:25Z

We need to keep optimizing the Flow parameter in the transact_info for better functionality. Rather than specifying the flow for a destination, we should specify the supported flows instead(for example, config a supported array for each chain). Most destination could support both flows.

In practice, this means that we will have separate extrinsics for each flow. The scheduleXcmpTask function would utilize the Normal flow, while scheduleXcmpTaskThroughProxy would utilize the Alternate flow. By distinguishing between the flows this way, we can achieve more efficient and targeted execution based on each task type’s requirements.

Furthermore, it's important to note that the current names, Normal and Alternate, lack clear meaning. The key distinction between them lies in who pays for the fees. In the Normal flow, Turing's sovereign account covers the XCM fee through ReserveAssetDeposited. On the other hand, in the Alternate flow, the user's proxy wallet bears the XCM fee. Another key difference is that the Alternate flow requires the user to pay XCM fees on the receiving end, thereby reducing the required amount of tokens on Turing (we only need to charge for task scheduling). However, whether users truly care about the slight fee difference remains uncertain.

You can find detailed instructions for each flow in the following code links:

Normal flow instructions
https://github.com/OAK-Foundation/OAK-blockchain/blob/67b4f7f5976d58dd35b080cf325eb01dff4921a7/pallets/xcmp-handler/src/lib.rs#L320

Alternate flow instructions
https://github.com/OAK-Foundation/OAK-blockchain/blob/67b4f7f5976d58dd35b080cf325eb01dff4921a7/pallets/xcmp-handler/src/lib.rs#L386C1-L386C1

While the name "Flow" is acceptable, it's worth considering more descriptive options since these sequences, combinations, or templates have distinct characteristics. I propose renaming the Flow selector to XcmTaskSupported, which consists of two types: scheduleWithPrepaidFees for the normal flow and scheduleWithoutPrepaidFees for the alternate flow. In both cases, the PrepaidFeeds refer to both the XCM fee and execution fee at the receiving end.

chrisli30 · 2023-07-03T01:26:44Z

Discussed the new flow parametes with Charles, we don’t need the chain storage XcmTaskSupported as of now, so after this PR there should be no XCM task relevant parameter left in chain storage.

…tinationAssetFeePerSecond

imstar15 · 2023-07-03T13:19:15Z

1, What is the TransactInfo before change? What are the keys and values?
2. What is the TransactInfo after? What are the keys and values?

We have removed TransactInfo storage.
User should input overallWeight and executionFee params for scheduleXcmpTask and scheduleXcmpTaskThroughProxy extrinsic.

overallWeight = encodedCallWeight + instructionWeight * instructionCount
executionFee =  FEE_PER_SECOND * overallWeight / WEIGHT_PER_SECOND

ScheduleXcmpTask run with PayThroughSovereignAccount flow, the execution fee for XCM instructions will be deducted in turing chain.
ScheduleXcmpTaskThroughProxy run with PayThroughRemoteDerivativeAccount flow, the execution fee for XCM instructions will not be deducted in turing chain.

If any extrinsic interface has changed, please provide screenshots for before and after.

Changes:

paraId -> destination: Multilation
currencyId -> scheduleFee
xcmAssetLocation -> executionFee, AssetPayment { asset_location, amount }
overallWeight, new Parameter

Before:

After:

chrisli30

A lot to improve for this code change.

pallets/automation-time/src/tests.rs

chrisli30 · 2023-07-07T18:02:55Z

pallets/automation-time/src/mock.rs

@@ -50,6 +50,9 @@ pub const NATIVE: CurrencyId = 0;
 pub const NATIVE_LOCATION: MultiLocation = MultiLocation { parents: 0, interior: Here };
 pub const FOREIGN_CURRENCY_ID: CurrencyId = 1;

+pub const FEE_PER_SECOND: u128 = 100_000_000_000;
+pub const XCMP_TASK_REF_TIME: u128 = 20_000;


You need to map the fee_per_second to destination, so we have

dest1: {
weight,
refTime,
assets: [ {token 1, fee per second 1}, {token 2, fee per second 2} ]
}

dest2 ..

and test them.

pallets/automation-time/src/tests.rs

chrisli30 · 2023-07-07T18:19:14Z

pallets/automation-time/src/tests.rs

+			AutomationTime::calculate_schedule_fee_amount(&action, execution_count)
+				.expect("Calculate schedule fee amount should work");
+		let expected_schedule_fee_amount =
+			FEE_PER_SECOND * XCMP_TASK_REF_TIME * (execution_count as u128) /


action is what you passed in to the function calculate_schedule_fee_amount. You need to use the variable values within action for calculation, instead of using a separate const variable XCMP_TASK_REF_TIME. XCMP_TASK_REF_TIME is not used in mock.

WEIGHT_REF_TIME_PER_SECOND is a global variable for our chain, so that’s the only constant variable which can be used in this line of validation.

Fee per second is in relation to each destination, you need to mock and to get them by destination.fee_per_second.

If the task is a xcmp task. the weight of the task is a fixed variable.

It is defined here.

https://github.com/OAK-Foundation/OAK-blockchain/blob/69464509e9dd628485b6633df1b33718ce42cbab/pallets/automation-time/src/weights.rs#L311

It is not calculated based on the data of the task/action.

So, I think it should be fine to define the XCMP_TASK_REF_TIME constant as well.

pallets/automation-time/src/tests.rs

v9n · 2023-07-10T20:05:09Z

pallets/automation-time/src/fees.rs

 		Ok(())
 	}

 	/// Withdraw the fee.
 	fn withdraw_fee(&self) -> Result<(), DispatchError> {
-		let fee = self.execution_fee.saturating_add(self.xcmp_fee);
+		let fee = self.schedule_fee_amount.saturating_add(self.execution_fee_amount);


let fee = self.schedule_fee_amount.saturating_add(self.execution_fee_amount);

we used this line in 2 places can_pay_fee and withdraw_fee . even though it's one liner I think it increase the chance of us accidently forgetting to keep sync that logic between the two. Instead, we may turn that single line into a method get_final_fee or some similar name, to encapsulate the logic of how we calculate the final fee

We will modify the FeeHandler logic in a later version, so don't worry about it in this PR.

chrisli30 · 2023-07-12T00:52:58Z

pallets/automation-time/src/tests.rs

-			overall_weight: Weight::from_ref_time(200_000),
-			schedule_as: Some(delegator_account),
-			flow: InstructionSequence::PayThroughRemoteDerivativeAccount,
+		let num_of_execution = 2;


This function is important. Could we use an arbitrary number such as 15, instead of 2? 2 is too common.

Same applies to the below num_of_execution values. Maybe just use a random value from 1 to 20.

chrisli30

I have a couple of minor suggestions but apply to many tests.

chrisli30 · 2023-07-12T00:53:34Z

pallets/automation-time/src/tests.rs

-			overall_weight: Weight::from_ref_time(200_000),
-			schedule_as: Some(delegator_account),
-			flow: InstructionSequence::PayThroughRemoteDerivativeAccount,
+		let num_of_execution = 2;


Same applies to the below num_of_execution values. Maybe just use a random value from 1 to 20.

chrisli30 · 2023-07-12T00:56:13Z

pallets/automation-time/src/tests.rs

+			),
+			..XcmpTaskParams::default()
+		});
+		let fee_amount_1 = AutomationTime::calculate_schedule_fee_amount(&action, num_of_execution)


Please use fee_amount_absolute instead of fee_amount_1. The same goes to all _1 and _2s. What are 1 and 2?

v9n · 2023-07-12T02:41:48Z

pallets/automation-time/src/lib.rs

-			para_id: ParaId,
-			currency_id: T::CurrencyId,
-			xcm_asset_location: VersionedMultiLocation,
+			destination: Box<VersionedMultiLocation>,


@imstar15 could you help add a comment on top of this because they are changed now? they used to be this:

375 /// Schedule a task through XCMP to fire an XCMP message with a provided call. 376 /// 377 /// Before the task can be scheduled the task must past validation checks. 378 /// * The transaction is signed 379 /// * The provided_id's length > 0 380 /// * The times are valid 381 /// * The given asset location is supported 382 /// 383 /// # Parameters 384 /// * `provided_id`: An id provided by the user. This id must be unique for the user. 385 /// * `execution_times`: The list of unix standard times in seconds for when the task should run. 386 /// * `para_id`: Parachain id the XCMP call will be sent to. 387 /// * `currency_id`: The currency in which fees will be paid. 388 /// * `encoded_call`: Call that will be sent via XCMP to the parachain id provided. 389 /// * `encoded_call_weight`: Required weight at most the provided call will take.

Okay, I will update the comment

I have updated the comment. Please review.

… function

chrisli30

Looks good to me!

v9n

lgtm

OAK-blockchain PR: AvaProtocol/OAK-blockchain#366

imstar15 force-pushed the xcmp-fee branch from 7126b66 to dc59cd3 Compare June 15, 2023 09:51

imstar15 mentioned this pull request Jun 30, 2023

Fix xcmp fee AvaProtocol/xcm-demo#71

Merged

imstar15 changed the title ~~[WIP]Fix xcmp_fee for automation-time~~ Fix xcmp_fee for automation-time Jun 30, 2023

imstar15 requested a review from chrisli30 June 30, 2023 08:46

imstar15 marked this pull request as ready for review June 30, 2023 08:47

chrisli30 requested changes Jul 1, 2023

View reviewed changes

chrisli30 requested a review from v9n July 1, 2023 00:54

imstar15 added 15 commits July 3, 2023 12:23

Add asset_location param to get_local_currency_instructions function

2ec380a

Add Convert<MultiLocation, Option<TokenId>> to automationTime

5ff1c93

Deduct fees based on different currency ID

34748a6

Split DestinationAssetConfig into TransactInfoWithWeightLimit and Des…

d86c890

…tinationAssetFeePerSecond

Change the type of destination to VersionedMultiLocation

5655f79

Modify Mangata asset config

8de84b6

Customize XCMP fee and weight.

d549846

Fix typo

d1698e2

Calculate xcmp fee for alternate flow

b90f278

Remove xcmp fee and weight calculation

8d0318e

Fix benchmark and tests

307514e

Fixing failed tests

7907b84

Add migration

02b9b7e

Code format

24a9e9b

Fix schedule_xcmp_task_full test

08c7d3c

imstar15 force-pushed the xcmp-fee branch from 6f99af0 to 08c7d3c Compare July 3, 2023 04:23

imstar15 added 2 commits July 3, 2023 20:50

Remove TransactInfo storage

0c308e8

Rename XcmFlow to XcmTaskSupported

de05ef0

imstar15 force-pushed the xcmp-fee branch from cbe56ee to de05ef0 Compare July 3, 2023 12:57

imstar15 added 3 commits July 7, 2023 16:30

Merge branch 'master' into xcmp-fee

f159237

Fix tests for the merged code

7181568

Add calculate_schedule_fee_amount_works test

617511c

imstar15 requested review from chrisli30 and v9n July 7, 2023 15:24

chrisli30 requested changes Jul 7, 2023

View reviewed changes

imstar15 and others added 3 commits July 8, 2023 11:28

Merge branch 'master' into xcmp-fee

69b8455

Add calculate_schedule_fee_amount tests

c87f79d

fixup: auto format Rust code

51eca0a

imstar15 changed the title ~~Fix xcmp_fee for automation-time~~ [WIP]Fix xcmp_fee for automation-time Jul 10, 2023

v9n reviewed Jul 10, 2023

View reviewed changes

pallets/automation-time/src/tests.rs Outdated Show resolved Hide resolved

v9n reviewed Jul 10, 2023

View reviewed changes

Add tests for all types of automation-time action

ab435de

imstar15 requested a review from v9n July 11, 2023 14:22

chrisli30 reviewed Jul 12, 2023

View reviewed changes

chrisli30 requested changes Jul 12, 2023

View reviewed changes

Set num_of_execution to a random number

e1c1e0e

imstar15 changed the title ~~[WIP]Fix xcmp_fee for automation-time~~ Fix xcmp_fee for automation-time Jul 12, 2023

imstar15 requested a review from chrisli30 July 12, 2023 02:32

v9n reviewed Jul 12, 2023

View reviewed changes

Add comment for schedule_xcmp_task , schedule_xcmp_task_through_proxy…

f4f6132

… function

imstar15 requested a review from v9n July 12, 2023 02:59

chrisli30 approved these changes Jul 12, 2023

View reviewed changes

v9n approved these changes Jul 12, 2023

View reviewed changes

imstar15 merged commit 54272be into master Jul 12, 2023

imstar15 deleted the xcmp-fee branch July 12, 2023 13:14

imstar15 added a commit to AvaProtocol/xcm-demo that referenced this pull request Jul 12, 2023

Fix xcmp fee (#71)

d537515

OAK-blockchain PR: AvaProtocol/OAK-blockchain#366

imstar15 mentioned this pull request Nov 29, 2023

Remove xcmp_handler in benchmark workflow #468

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix xcmp_fee for automation-time #366

Fix xcmp_fee for automation-time #366

imstar15 commented Jun 9, 2023

chrisli30 left a comment

chrisli30 commented Jul 1, 2023

chrisli30 commented Jul 1, 2023 •

edited

Loading

chrisli30 commented Jul 3, 2023

imstar15 commented Jul 3, 2023 •

edited

Loading

chrisli30 left a comment

chrisli30 Jul 7, 2023

chrisli30 Jul 7, 2023

chrisli30 Jul 7, 2023

imstar15 Jul 10, 2023 •

edited

Loading

v9n Jul 10, 2023 •

edited

Loading

imstar15 Jul 11, 2023 •

edited

Loading

chrisli30 Jul 12, 2023

chrisli30 Jul 12, 2023

chrisli30 left a comment

chrisli30 Jul 12, 2023

chrisli30 Jul 12, 2023 •

edited

Loading

v9n Jul 12, 2023

imstar15 Jul 12, 2023

imstar15 Jul 12, 2023

chrisli30 left a comment

v9n left a comment

Fix xcmp_fee for automation-time #366

Fix xcmp_fee for automation-time #366

Conversation

imstar15 commented Jun 9, 2023

chrisli30 left a comment

Choose a reason for hiding this comment

chrisli30 commented Jul 1, 2023

chrisli30 commented Jul 1, 2023 • edited Loading

chrisli30 commented Jul 3, 2023

imstar15 commented Jul 3, 2023 • edited Loading

chrisli30 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imstar15 Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

v9n Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

imstar15 Jul 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisli30 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisli30 Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisli30 left a comment

Choose a reason for hiding this comment

v9n left a comment

Choose a reason for hiding this comment

chrisli30 commented Jul 1, 2023 •

edited

Loading

imstar15 commented Jul 3, 2023 •

edited

Loading

imstar15 Jul 10, 2023 •

edited

Loading

v9n Jul 10, 2023 •

edited

Loading

imstar15 Jul 11, 2023 •

edited

Loading

chrisli30 Jul 12, 2023 •

edited

Loading