Asset Conversion: Pool Touch Call #3251

muharem · 2024-02-08T06:56:01Z

Introduce touch call designed to address operational prerequisites before providing liquidity to a pool.

This function ensures that essential requirements, such as the presence of the pool's accounts, are fulfilled. It is particularly beneficial in scenarios where a pool creator removes the pool's accounts without providing liquidity.

…rsion-pool-touch

muharem · 2024-03-13T12:31:23Z

/bot help

muharem · 2024-03-13T13:38:54Z

bot help

command-bot · 2024-03-13T13:38:57Z

Here's a link to docs

muharem · 2024-03-13T13:40:42Z

bot bench substrate-pallet --pallet=pallet-asset-conversion

command-bot · 2024-03-13T13:40:45Z

@muharem option '--pallet ' argument 'pallet-asset-conversion' is invalid. argument pallet is not matching rule ^([a-z_]+)([:]{2}[a-z_]+)?$

muharem · 2024-03-13T13:41:06Z

bot bench substrate-pallet --pallet=pallet_asset_conversion

command-bot · 2024-03-13T13:41:12Z

@muharem https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5516365 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --pallet=pallet_asset_conversion. Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 6-4abc20ff-0039-47b2-ac9c-7f463ff8f1e2 to cancel this command or bot cancel to cancel all commands in this pull request.

…=dev --target_dir=substrate --pallet=pallet_asset_conversion

command-bot · 2024-03-13T14:14:19Z

@muharem Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --pallet=pallet_asset_conversion has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5516365 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5516365/artifacts/download.

…rsion-pool-touch

georgepisaltu

Overall looks good, a couple of comments.

georgepisaltu · 2024-03-25T17:35:59Z

substrate/frame/support/src/traits/tokens/fungibles/lifetime.rs

@@ -32,6 +32,18 @@ pub trait Create<AccountId>: Inspect<AccountId> {
 	) -> DispatchResult;
 }

+/// Trait for refunding the deposit of a target asset account.
+pub trait Refund<AccountId> {


Shouldn't this be trait Refund<AccountId>: Inspect<AccountId> to avoid defining AssetId and Balance again?

I think it is more flexible, refund type do not have to implement the inspect trait.

Is there ever a usecase where Refund would be implemented on something other than the thing implementing Inspect. Also, is there ever a usecase where Refund has a different AssetId and/or Balance type than the underlying Inspect fungibles impl? I think removing this type complexity is nice.

Also, all of the other traits in this file use Inspect as a base. Same goes for the other traits in this module.

Is there ever a usecase where Refund would be implemented on something other than the thing implementing Inspect.

Not likely in the context of the FRAME. I really see the point for super traits when some of the functions/methods used in a sub trait. But when need to decide convenience (no needs for type definition) vs flexibility (interface segregation), I will go for flexibility.
In this case it's not that important, perhaps Refund wont need to be implemented on a type not implementing Inspect. I am fine with super trait.

@georgepisaltu I had to set an additional bound to Currency type in Assets pallet for this. Please check and approve if you happy with it.
290c0f1

however I do not see the benefits. in most cases you still have to define what Balance and AssetId are. and now you can not have a separate type implementing only refund.

georgepisaltu · 2024-03-25T17:39:52Z

substrate/frame/support/src/traits/tokens/fungibles/lifetime.rs

+	/// Scalar type for representing balance of an account.
+	type Balance: Balance;
+	/// Returns the amount of account deposit and depositor address, if any.
+	fn deposit_held(id: Self::AssetId, who: AccountId) -> Option<(AccountId, Self::Balance)>;


I think this function is unnecessary in the Refund interface. Having fn refund should be enough, the other fungibles trait impls can be used to determine the deposits.

fungibles traits have no function to check the deposit held for an asset's account existence.

I was saying that fn deposit_held is unnecessary in Refund because:

the usage below in fn refund doesn't use the Balance parameter; in fact, fn refund only cares about whether the caller is the depositor or if it's someone else (or no one)

aside from the use above which can clearly be refactored out, it's only used in benchmarks; we wouldn't even need to keep it with a benchmark feature gate because you would have access to the Assets pallet storage in the bench to check the deposit, but you could keep it around as a helper or something

fungibles traits have no function to check the deposit held for an asset's account existence.

Again, seems it's unused functionality, but IMO even if fungibles don't let you check this deposit, it doesn't feel like this functionality should belong in a Refund trait. Maybe I'm missing something.

Here is the example where it is used
https://github.com/paritytech/polkadot-sdk/pull/3250/files#diff-fbf88fab34bcf066e3c722cd1345365529c1c652df82604ba23aea4f047993d5R243

polkadot-sdk/substrate/frame/asset-conversion/ops/src/lib.rs

Line 182 in 05e639d

T::AssetsRefund::deposit_held(asset1.clone(), prior_account.clone())

I was indeed missing something because that's from another PR 😁 .

I still feel this functionality for querying the deposit shouldn't be in a Refund trait, but rather in some other parent interface to separate these concerns. In practical terms though, it doesn't matter too much. In the future, if we need to separate them, it will be very easy to do so.

Just querying the refund amount held can also be done with a Get<Option<(AssetID, AccountId)>>.

In this case types like pallet, will implement a general Get trait, might be not enough to differentiate from other impls. I think the current way is better

georgepisaltu · 2024-03-25T17:41:41Z

substrate/frame/assets/src/impl_fungibles.rs

+		}
+	}
+	fn refund(id: Self::AssetId, who: T::AccountId) -> DispatchResult {
+		match Self::deposit_held(id.clone(), who.clone()) {


I think calling deposit_held here only complicates things, especially because you don't care about the actual value of the deposit at this level, only if it is provided by someone else. It would be simpler to move the logic from deposit_held above here.

I will duplicate that match and get from above. No strong opinion though

If my argument was not convincing and you decide against removing fn deposit_held, then there is no point in duplicating the logic here.

…rsion-pool-touch

substrate/frame/assets/src/lib.rs

This reverts commit 290c0f1.

This reverts commit ea87fe2.

github-actions · 2024-04-15T15:13:05Z

Review required! Latest push from author must always be reviewed

ggwpez · 2024-04-16T10:36:21Z

substrate/frame/support/src/traits/tokens/fungible/union_of.rs

+
+impl<
+		Left: fungible::Inspect<AccountId>,
+		Right: fungibles::Inspect<AccountId> + fungibles::Refund<AccountId>,


Why is this adapter requiring heterogeneous types for Left and Right?
Why is the left side just assumed to not be Refund?

yes, we ether assume there is no refund for fungible, or we introduce a no-op impl for fungible, which is balance pallet, with asset id (), or we have to introduce one more refund trait without asset id. Given this, I made a chose for no-op in adapter (no refund trait for fungible, no adapter for it). I am open to go for another solution, as I do not see an obvious winner here.

I doubt we will need refund for fungible. I would prefer to introduce it if we need it in the future.

…rsion-pool-touch

Introduce `touch` call designed to address operational prerequisites before providing liquidity to a pool. This function ensures that essential requirements, such as the presence of the pool's accounts, are fulfilled. It is particularly beneficial in scenarios where a pool creator removes the pool's accounts without providing liquidity. --------- Co-authored-by: command-bot <>

muharem added 4 commits February 8, 2024 14:34

refund trait and impls

5155e94

touch call

8889af9

weights for runtimes

7b8503c

docs

8c99ad6

muharem added the T2-pallets This PR/Issue is related to a particular pallet. label Feb 8, 2024

muharem requested a review from a team as a code owner February 8, 2024 06:56

muharem added 4 commits February 8, 2024 14:58

prdocs

405a4b9

should touch

f448674

clippy fix

e8fbdaa

Merge remote-tracking branch 'origin/master' into muharem-asset-conve…

3ff9a10

…rsion-pool-touch

".git/.scripts/commands/bench/bench.sh" --subcommand=pallet --runtime…

052c270

…=dev --target_dir=substrate --pallet=pallet_asset_conversion

Merge remote-tracking branch 'origin/master' into muharem-asset-conve…

4cdc814

…rsion-pool-touch

muharem requested review from NachoPal, joepetrowski and georgepisaltu March 13, 2024 15:21

georgepisaltu reviewed Mar 25, 2024

View reviewed changes

muharem added 2 commits April 15, 2024 13:57

Merge remote-tracking branch 'origin/master' into muharem-asset-conve…

ab196b6

…rsion-pool-touch

update refund trait

290c0f1

georgepisaltu approved these changes Apr 15, 2024

View reviewed changes

remove unused imports

ea87fe2

ggwpez reviewed Apr 15, 2024

View reviewed changes

substrate/frame/assets/src/lib.rs Show resolved Hide resolved

paritytech-review-bot bot requested a review from a team April 15, 2024 13:15

muharem added 2 commits April 15, 2024 17:12

Revert "update refund trait"

0b025b3

This reverts commit 290c0f1.

Revert "remove unused imports"

8172344

This reverts commit ea87fe2.

github-actions bot requested a review from georgepisaltu April 15, 2024 15:13

update doc

2b93d90

georgepisaltu approved these changes Apr 15, 2024

View reviewed changes

ggwpez reviewed Apr 16, 2024

View reviewed changes

ggwpez approved these changes Apr 16, 2024

View reviewed changes

Merge remote-tracking branch 'origin/master' into muharem-asset-conve…

37d06ee

…rsion-pool-touch

muharem added this pull request to the merge queue Apr 17, 2024

Merged via the queue into master with commit 305d311 Apr 17, 2024
132 of 135 checks passed

muharem deleted the muharem-asset-conversion-pool-touch branch April 17, 2024 17:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Asset Conversion: Pool Touch Call #3251

Asset Conversion: Pool Touch Call #3251

muharem commented Feb 8, 2024

muharem commented Mar 13, 2024

muharem commented Mar 13, 2024

command-bot bot commented Mar 13, 2024

muharem commented Mar 13, 2024

command-bot bot commented Mar 13, 2024

muharem commented Mar 13, 2024

command-bot bot commented Mar 13, 2024 •

edited

Loading

command-bot bot commented Mar 13, 2024

georgepisaltu left a comment

georgepisaltu Mar 25, 2024

muharem Mar 26, 2024

georgepisaltu Mar 26, 2024

muharem Mar 27, 2024

muharem Apr 15, 2024

muharem Apr 15, 2024 •

edited

Loading

georgepisaltu Mar 25, 2024

muharem Mar 26, 2024

georgepisaltu Mar 26, 2024

muharem Mar 27, 2024

georgepisaltu Mar 27, 2024

ggwpez Apr 15, 2024

muharem Apr 15, 2024

georgepisaltu Mar 25, 2024

muharem Mar 26, 2024

georgepisaltu Mar 26, 2024

github-actions bot commented Apr 15, 2024

ggwpez Apr 16, 2024

muharem Apr 16, 2024

muharem Apr 16, 2024

Asset Conversion: Pool Touch Call #3251

Asset Conversion: Pool Touch Call #3251

Conversation

muharem commented Feb 8, 2024

muharem commented Mar 13, 2024

muharem commented Mar 13, 2024

command-bot bot commented Mar 13, 2024

muharem commented Mar 13, 2024

command-bot bot commented Mar 13, 2024

muharem commented Mar 13, 2024

command-bot bot commented Mar 13, 2024 • edited Loading

command-bot bot commented Mar 13, 2024

georgepisaltu 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muharem Apr 15, 2024 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

command-bot bot commented Mar 13, 2024 •

edited

Loading

muharem Apr 15, 2024 •

edited

Loading