Refactor transaction storage pallet to use fungible traits #1800

acatangiu · 2023-10-05T14:53:37Z

Partial #226

frame/transaction-storage: replace Currency with fungible::* traits

Signed-off-by: Adrian Catangiu <adrian@parity.io>

substrate/frame/transaction-storage/src/lib.rs

liamaharon · 2023-10-07T21:05:47Z

substrate/frame/transaction-storage/src/lib.rs

+			T::Currency::hold(&HoldReason::StorageFeeHold.into(), &sender, fee)?;
+			let (credit, _) = T::Currency::slash(&HoldReason::StorageFeeHold.into(), &sender, fee);


Seems logically equivalent to just reducing the account balance by fee if the balance is gt fee (which seems way simpler)? Or am I missing something?

Not sure if equivalent, the imbalance/credit does need to go to T::FeeDestination, but maybe this can be changed too.

@paritytech/frame-coders may have a stronger opinion which way to go here.

You could very well use decrease_balance() which returns the reduced balance. I don't see why that wouldn't work.
Though, I don't know why it was not used in the original code

Disregard my previous comment, I think @acatangiu you are right, we do want slash to manage the imbalance and T::FeeDestination to do whatever it needs to be done for us.

decrease_balance() should not be used directly for that reason.

polkadot-sdk/substrate/frame/support/src/traits/tokens/fungible/regular.rs

Lines 122 to 127 in 21d36b7

/// **WARNING**

/// Do not use this directly unless you want trouble, since it allows you to alter account balances

/// without keeping the issuance up to date. It has no safeguards against accidentally creating

/// token imbalances in your system leading to accidental inflation or deflation. It's really just

/// for the underlying datatype to implement so the user gets the much safer `Balanced` trait to

/// use.

…e-refactor-transaction-storage-pallet

juangirini

LGTM just a few optional nits

substrate/frame/transaction-storage/src/benchmarking.rs

substrate/frame/transaction-storage/src/lib.rs

substrate/frame/transaction-storage/src/mock.rs

…e-refactor-transaction-storage-pallet

acatangiu · 2023-10-30T10:28:06Z

@liamaharon your comments are addressed, please take another look 🙏

paritytech-cicd-pr · 2023-10-30T11:00:21Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 1/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4124271

…ld reason

substrate/frame/transaction-storage/src/lib.rs

Co-authored-by: georgepisaltu <52418509+georgepisaltu@users.noreply.github.com>

…e-refactor-transaction-storage-pallet

gupnik · 2023-10-31T06:39:06Z

@acatangiu Any reason why Consideration was not used here? Seems perfectly suitable to this use-case.

acatangiu · 2023-10-31T10:16:05Z

@acatangiu Any reason why Consideration was not used here? Seems perfectly suitable to this use-case.

I'm not familiar with Consideration so that answers your question :) also goes to say I can't tell if it's better/worse.
Feel free to open a follow-up PR for it if you think it's more suited.

Polkadot-Forum · 2023-12-06T22:53:18Z

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-release-analysis-v1-4-0/5152/1

…ch#1800) Partial paritytech#226 `frame/transaction-storage`: replace `Currency` with `fungible::*` traits --------- Signed-off-by: Adrian Catangiu <adrian@parity.io> Co-authored-by: georgepisaltu <52418509+georgepisaltu@users.noreply.github.com>

acatangiu self-assigned this Oct 5, 2023

acatangiu mentioned this pull request Oct 5, 2023

FRAME: Move pallets over to use fungible traits #226

Open

33 tasks

acatangiu force-pushed the frame-refactor-transaction-storage-pallet branch 2 times, most recently from 4a72011 to 9aa58ea Compare October 7, 2023 14:33

acatangiu marked this pull request as ready for review October 7, 2023 14:33

acatangiu requested review from a team October 7, 2023 14:33

acatangiu added the T2-pallets This PR/Issue is related to a particular pallet. label Oct 7, 2023

frame/transaction-storage: replace Currency with fungible traits

362e261

Signed-off-by: Adrian Catangiu <adrian@parity.io>

acatangiu force-pushed the frame-refactor-transaction-storage-pallet branch from 9aa58ea to 362e261 Compare October 7, 2023 15:18

liamaharon reviewed Oct 7, 2023

View reviewed changes

paritytech-ci requested a review from a team October 7, 2023 21:16

acatangiu added 3 commits October 10, 2023 13:24

Merge branch 'master' of github.com:paritytech/polkadot-sdk into fram…

0bf9b61

…e-refactor-transaction-storage-pallet

address review comments

00e6fc7

fix benchmarks

c10caac

juangirini approved these changes Oct 26, 2023

View reviewed changes

acatangiu added 3 commits October 30, 2023 11:43

Merge branch 'master' of github.com:paritytech/polkadot-sdk into fram…

d716906

…e-refactor-transaction-storage-pallet

fix merge damage

820a279

address review comments

6c0c9c4

acatangiu added 2 commits October 30, 2023 13:12

kitchensink-runtime: bump MaxHolds to account for newly introduced ho…

e8bf46a

…ld reason

fix incomplete rename

17addaa

georgepisaltu approved these changes Oct 30, 2023

View reviewed changes

substrate/frame/transaction-storage/src/lib.rs Outdated Show resolved Hide resolved

acatangiu and others added 3 commits October 30, 2023 13:59

Update substrate/frame/transaction-storage/src/lib.rs

11fa7ca

Co-authored-by: georgepisaltu <52418509+georgepisaltu@users.noreply.github.com>

fix warning

5248376

Merge branch 'master' of github.com:paritytech/polkadot-sdk into fram…

27f6999

…e-refactor-transaction-storage-pallet

acatangiu merged commit 30f3ad2 into paritytech:master Oct 30, 2023
109 of 110 checks passed

acatangiu deleted the frame-refactor-transaction-storage-pallet branch October 30, 2023 13:15

github-actions bot mentioned this pull request Feb 19, 2024

Update substrate/polkadot/cumulus from v1.3.0 to v1.6.0 moondance-labs/tanssi#419

Closed

github-actions bot mentioned this pull request Mar 13, 2024

Update polkadot-sdk from v1.3.0 to v1.7.2 moonbeam-foundation/moonbeam#2703

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor transaction storage pallet to use fungible traits #1800

Refactor transaction storage pallet to use fungible traits #1800

acatangiu commented Oct 5, 2023 •

edited by liamaharon

Loading

liamaharon Oct 7, 2023 •

edited

Loading

acatangiu Oct 10, 2023

juangirini Oct 26, 2023 •

edited

Loading

juangirini left a comment

acatangiu commented Oct 30, 2023

paritytech-cicd-pr commented Oct 30, 2023

gupnik commented Oct 31, 2023

acatangiu commented Oct 31, 2023

Polkadot-Forum commented Dec 6, 2023

		T::Currency::hold(&HoldReason::StorageFeeHold.into(), &sender, fee)?;
		let (credit, _) = T::Currency::slash(&HoldReason::StorageFeeHold.into(), &sender, fee);

	/// WARNING
	/// Do not use this directly unless you want trouble, since it allows you to alter account balances
	/// without keeping the issuance up to date. It has no safeguards against accidentally creating
	/// token imbalances in your system leading to accidental inflation or deflation. It's really just
	/// for the underlying datatype to implement so the user gets the much safer `Balanced` trait to
	/// use.

Refactor transaction storage pallet to use fungible traits #1800

Refactor transaction storage pallet to use fungible traits #1800

Conversation

acatangiu commented Oct 5, 2023 • edited by liamaharon Loading

liamaharon Oct 7, 2023 • edited Loading

Choose a reason for hiding this comment

acatangiu Oct 10, 2023

Choose a reason for hiding this comment

juangirini Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

juangirini left a comment

Choose a reason for hiding this comment

acatangiu commented Oct 30, 2023

paritytech-cicd-pr commented Oct 30, 2023

gupnik commented Oct 31, 2023

acatangiu commented Oct 31, 2023

Polkadot-Forum commented Dec 6, 2023

acatangiu commented Oct 5, 2023 •

edited by liamaharon

Loading

liamaharon Oct 7, 2023 •

edited

Loading

juangirini Oct 26, 2023 •

edited

Loading