CodeSee Demo Review Fork #3

mattheworris · 2023-12-04T20:50:12Z

Goal

The goal of this PR is to replace the Currency trait with the fungible trait.

Closes frequency-chain#942
Closes frequency-chain#1532

Discussion

The following Parity issues/PRs were used as references for changes:
Deprecate Currency - PR 12951 (Explanation of necessary changes.)
FRAME: Move pallets over to use fungible traits (Issue to track Parity's efforts to update their pallets.)
pallet vesting / update to use fungible (Example of some necessary changes.)

Changes

.cargo-deny.toml Added "multiformats" for cid crate to fix warning.
vscode/settings.json Added script that sets up the source map for the rust std library files for debugging.
Cargo.lock cargo updated.
e2e/package-lock.json npm updated.
Replaced traits as needed: Use tokens::fungible::
- InspectFungible for balance() and reducible_balance()
- InspectFreeze for balance_frozen()
- Mutate for set_balance(), mint_into()
- MutateFreeze for set_freeze(), and thaw()
Added pub enum FreezeReason to support freezes
Updated error handling as set_freeze() and thaw() can fail, so errors needed to be propagated.
Updated mocks to set MaxLocks, MaxReserves, MaxFreezes, and MaxHolds to 50, to match Balances pallet config.
Updated runtime pallet configs to use BalancesMaxXXXXXs
Updated tests with .expect() where set_freeze() or thaw() can fail.
FreezeIdentifier and RuntimeFreezeReason configured with defaults.

How to Review

Read through Deprecate Currency - PR 12951 to understand context and check that Currency traits were properly replaced with fungible traits.
Check impact of changing to set_freeze() and thaw() which can now fail and make sure all error states are propagated correctly without possibility for panic
Check if balance() is used correctly, or should be changed to reducible_balance(). The calculations evaluating the Existential Deposit (ED) have been updated and Parity comments indicate that reducible_balance() is most likely the value needed.

Checklist

…e maps;

…rors for T::Currency::set_freeze/thaw;

…unt()

…942-capacity-replace-currency-trait-with-fungible-trait

ghost · 2023-12-04T21:12:06Z

👇 Click on the image for a new way to code review

Legend

mattheworris · 2023-12-05T18:23:19Z

pallets/capacity/src/lib.rs

@@ -100,6 +101,16 @@ pub mod pallet {
 	};
 	use sp_runtime::traits::{AtLeast32BitUnsigned, MaybeDisplay};

+	/// A reason for freezing funds.
+	#[pallet::composite_enum]
+	pub enum FreezeReason {


FreezeReason is required for the fungible traits. The enum is defined and used by capacity and time-release. Comments in the Parity deprecation pr indicated that there should only be one enum:

Both Holds and Freezes require an identifier which is configurable and is expected to be an enum aggregated across all pallet instances of the runtime.

This enum is scoped to each pallet, but we may want to move it to a common location within the runtime, if possible.

mattheworris · 2023-12-05T18:26:22Z

pallets/capacity/src/lib.rs

-		/// Function that allows a balance to be locked.
-		type Currency: LockableCurrency<Self::AccountId, Moment = BlockNumberFor<Self>>;
+		/// Functions that allow a fungible balance to be changed or frozen.
+		type Currency: MutateFreeze<Self::AccountId, Id = Self::RuntimeFreezeReason>


Updating Frequency's type Currency with the traits from frame_support::traits::tokens::fungible

mattheworris · 2023-12-05T18:46:43Z

pallets/capacity/src/lib.rs

@@ -562,7 +579,7 @@ impl<T: Config> Pallet<T> {
 		staker: &T::AccountId,
 		proposed_amount: BalanceOf<T>,
 	) -> BalanceOf<T> {
-		let account_balance = T::Currency::free_balance(&staker);
+		let account_balance = T::Currency::balance(&staker);


Some fungible methods require name changes: free_balance -> balance

mattheworris · 2023-12-05T18:50:20Z

pallets/capacity/src/lib.rs

@@ -589,9 +606,9 @@ impl<T: Config> Pallet<T> {
 		let staking_account = Self::get_staking_account_for(staker).unwrap_or_default();
 		let total_locked = staking_account.active.saturating_add(total_unlocking);
 		if total_locked.is_zero() {
-			T::Currency::remove_lock(STAKING_ID, &staker);
+			T::Currency::thaw(&FreezeReason::Staked.into(), staker)?;


remove_lock was infallible and did not return anything. thaw returns a result, so do_withdraw_unstaked needs to propagate any errors.

mattheworris · 2023-12-05T18:58:47Z

pallets/time-release/src/lib.rs

+		type RuntimeFreezeReason: From<FreezeReason>;
+
+		/// We need MaybeSerializeDeserialize because of the genesis config.
+		type Balance: Balance + MaybeSerializeDeserialize;


In time-release, the Frequency type Balance needed to be updated to include the serialize/deserialize traits.

mattheworris · 2023-12-05T19:00:28Z

runtime/frequency/src/lib.rs

@@ -467,6 +468,7 @@ pub type MaxReleaseSchedules = ConstU32<{ MAX_RELEASE_SCHEDULES }>;
 // the descriptions of these configs.
 impl pallet_time_release::Config for Runtime {
 	type RuntimeEvent = RuntimeEvent;
+	type Balance = Balance;


Here we update type Balance because time-release adds the MaybeSerializeDeserialize trait.

…942-capacity-replace-currency-trait-with-fungible-trait

mattheworris · 2023-12-06T18:10:31Z

pallets/capacity/src/lib.rs

+		#[codec(index = 0)]
+		Staked,
+		/// Funds are currently locked and are not yet liquid.
+		NotYetVested,


Remove this value if enum stays in this pallet.

mattheworris · 2023-12-06T18:21:45Z

pallets/capacity/src/tests/withdraw_unstaked_tests.rs

-		assert_eq!(14u64, Balances::locks(&staker)[0].amount);
+		assert_eq!(
+			14u64,
+			<Test as Config>::Currency::balance_frozen(&FreezeReason::Staked.into(), &staker)


Check to see if line can be shorter by removing Test as Config.

mattheworris · 2023-12-06T18:26:32Z

pallets/time-release/src/lib.rs

 						"Account do not have enough balance"
 					);

-					T::Currency::set_lock(
-						RELEASE_LOCK_ID,
+					let _ = T::Currency::set_freeze(


Handle Result.

mattheworris · 2023-12-06T18:43:26Z

runtime/frequency/src/lib.rs

-	type MaxHolds = ConstU32<0>;
-	type MaxFreezes = ConstU32<0>;
+	type MaxHolds = BalancesMaxHolds;
+	type MaxFreezes = BalancesMaxFreezes;


Need more research on Locks/Freezes and how they affect time-release pallet, etc.

Matthew Orris and others added 25 commits November 17, 2023 16:21

chore: Replace Currency->fungible: update pallet_capacity with fungible

f95a3c9

fix: keep Currency name and update derived trait

72543fa

fix: pallet_capacity tests working with correct types/traits

478ee54

fix: capacity pallet tests passing; update vscode with rust std sourc…

1befab1

…e maps;

fix: lint changes;

7fbdf01

fix: benchmarks updates for Currency trait

7eceac4

fix: time-release pallet updated with fungible traits; test working

1dad909

fix: time-release runtime-benchmarks errors

02f57a0

fix: update comments; rename FreezeReason::TimeReleased

225b345

fix: enum should be the same; updates;

57d7d94

fix: update cargo dependencies for common-runtime;

86c6e74

fix: Currency trait was incorrect;

bf228b5

fix: set_balance logic error;

72feddc

fix: revert new Currency type; set FreezeIdentifier in mock;

1442a12

fix: lints

cda4bc4

we are erroring silently - Number of freezes exceed MaxFreezes

d63a554

test pass

09261a6

lint

e855d93

fix: remove .only

6c4e44a

fix: coordinate BalancesMaxXXXXX with existing runtime defaults

9079373

fix: add expect to tests that can fail; cargo update; pass through er…

52d5ea4

…rors for T::Currency::set_freeze/thaw;

fix: capacity benchmarking should handle errors from set_staking_acco…

da2296f

…unt()

Merge branch 'main' of https://github.com/LibertyDSNP/frequency into …

50d5089

…942-capacity-replace-currency-trait-with-fungible-trait

fix: changes for errors from merge from main

568582b

fix: Update spec_version to 66

6052042

mattheworris self-assigned this Dec 4, 2023

mattheworris commented Dec 5, 2023

View reviewed changes

Matthew Orris added 3 commits December 5, 2023 14:58

Fix: thaw error handling in delete_lock function

9384ed3

Merge branch 'main' of https://github.com/LibertyDSNP/frequency into …

57cd323

…942-capacity-replace-currency-trait-with-fungible-trait

fix: Update spec_version to 67

c77dd41

mattheworris commented Dec 6, 2023

View reviewed changes

mattheworris force-pushed the main branch from 7b342a9 to b871aac Compare March 8, 2024 21:14

mattheworris closed this May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CodeSee Demo Review Fork #3

CodeSee Demo Review Fork #3

mattheworris commented Dec 4, 2023 •

edited

Loading

ghost commented Dec 4, 2023 •

edited by ghost

Loading

Legend

mattheworris Dec 5, 2023

mattheworris Dec 5, 2023

mattheworris Dec 5, 2023

mattheworris Dec 5, 2023

mattheworris Dec 5, 2023

mattheworris Dec 5, 2023

mattheworris Dec 6, 2023

mattheworris Dec 6, 2023

mattheworris Dec 6, 2023

mattheworris Dec 6, 2023

CodeSee Demo Review Fork #3

CodeSee Demo Review Fork #3

Conversation

mattheworris commented Dec 4, 2023 • edited Loading

Goal

Discussion

Changes

How to Review

Checklist

ghost commented Dec 4, 2023 • edited by ghost Loading

Legend

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

mattheworris commented Dec 4, 2023 •

edited

Loading

ghost commented Dec 4, 2023 •

edited by ghost

Loading