From 91d845686b1e9aabea8ce1e025152d8a6ec58ca6 Mon Sep 17 00:00:00 2001 From: Bryan Chen Date: Mon, 20 Sep 2021 15:50:14 +1200 Subject: [PATCH 1/4] review & cleanup --- pallets/asset-index/src/lib.rs | 56 ++++++++++--------------- pallets/committee/src/lib.rs | 26 ++++++------ pallets/remote-asset-manager/src/lib.rs | 6 ++- 3 files changed, 40 insertions(+), 48 deletions(-) diff --git a/pallets/asset-index/src/lib.rs b/pallets/asset-index/src/lib.rs index 40cb0eedb8..4c3f44254d 100644 --- a/pallets/asset-index/src/lib.rs +++ b/pallets/asset-index/src/lib.rs @@ -34,7 +34,6 @@ pub mod pallet { use primitives::traits::AssetRecorderBenchmarks; use frame_support::{ - dispatch::DispatchResultWithPostInfo, pallet_prelude::*, sp_runtime::{ traits::{AccountIdConversion, AtLeast32BitUnsigned, CheckedAdd, CheckedDiv, CheckedSub, Saturating, Zero}, @@ -360,7 +359,7 @@ pub mod pallet { units: T::Balance, location: MultiLocation, amount: T::Balance, - ) -> DispatchResultWithPostInfo { + ) -> DispatchResult { Self::do_add_asset(T::AdminOrigin::ensure_origin(origin)?, asset_id, units, location, amount) } @@ -373,7 +372,7 @@ pub mod pallet { location: MultiLocation, amount: T::Balance, recipient: T::AccountId, - ) -> DispatchResultWithPostInfo { + ) -> DispatchResult { ensure_root(origin)?; Self::do_add_asset(recipient, asset_id, units, location, amount) } @@ -394,7 +393,7 @@ pub mod pallet { asset_id: T::AssetId, units: T::Balance, recipient: Option, - ) -> DispatchResultWithPostInfo { + ) -> DispatchResult { Self::do_remove_asset(T::AdminOrigin::ensure_origin(origin)?, asset_id, units, recipient) } @@ -406,7 +405,7 @@ pub mod pallet { asset_id: T::AssetId, units: T::Balance, recipient: Option, - ) -> DispatchResultWithPostInfo { + ) -> DispatchResult { ensure_root(origin)?; Self::do_remove_asset(who, asset_id, units, recipient) } @@ -444,7 +443,7 @@ pub mod pallet { T::AdminOrigin::ensure_origin(origin)?; ensure!(!new_range.minimum.is_zero(), Error::::InvalidDepositRange); ensure!(new_range.maximum > new_range.minimum, Error::::InvalidDepositRange); - IndexTokenDepositRange::::put(new_range.clone()); + IndexTokenDepositRange::::put(&new_range); Self::deposit_event(Event::::IndexTokenDepositRangeUpdated(new_range)); Ok(()) } @@ -483,12 +482,9 @@ pub mod pallet { let bounded_symbol: BoundedVec = symbol.clone().try_into().map_err(|_| Error::::BadMetadata)?; - Metadata::::try_mutate_exists(id, |metadata| { - *metadata = Some(AssetMetadata { name: bounded_name, symbol: bounded_symbol, decimals }); - - Self::deposit_event(Event::MetadataSet(id, name, symbol, decimals)); - Ok(()) - }) + Metadata::::insert(id, AssetMetadata { name: bounded_name, symbol: bounded_symbol, decimals }); + Self::deposit_event(Event::MetadataSet(id, name, symbol, decimals)); + Ok(()) } /// Initiate a transfer from the user's sovereign account into the @@ -498,6 +494,7 @@ pub mod pallet { /// account and mints PINT proportionally using the latest /// available price pairs #[pallet::weight(T::WeightInfo::deposit())] + #[transactional] pub fn deposit(origin: OriginFor, asset_id: T::AssetId, units: T::Balance) -> DispatchResult { let caller = T::AdminOrigin::ensure_origin(origin)?; if units.is_zero() { @@ -530,12 +527,8 @@ pub mod pallet { T::RemoteAssetManager::deposit(asset_id, units); // insert new deposit - >::try_mutate(&caller, |deposits| -> DispatchResult { - deposits - .try_push((index_tokens, frame_system::Pallet::::block_number())) - .map_err(|_| Error::::TooManyDeposits)?; - Ok(()) - })?; + Deposits::::try_append(&caller, (index_tokens, frame_system::Pallet::::block_number())) + .map_err(|_| Error::::TooManyDeposits)?; Self::deposit_event(Event::Deposited(asset_id, units, caller, index_tokens)); Ok(()) @@ -554,7 +547,7 @@ pub mod pallet { /// ratio of the liquid assets in the index. #[pallet::weight(T::WeightInfo::withdraw())] #[transactional] - pub fn withdraw(origin: OriginFor, amount: T::Balance) -> DispatchResultWithPostInfo { + pub fn withdraw(origin: OriginFor, amount: T::Balance) -> DispatchResult { let caller = T::AdminOrigin::ensure_origin(origin.clone())?; ensure!(amount >= T::MinimumRedemption::get(), Error::::MinimumRedemption); @@ -613,13 +606,10 @@ pub mod pallet { // state let end_block = frame_system::Pallet::::block_number().saturating_add(T::WithdrawalPeriod::get()); // lock the assets for the withdrawal period starting at current block - PendingWithdrawals::::mutate(&caller, |maybe_redemption| { - let redemption = maybe_redemption.get_or_insert_with(|| Vec::with_capacity(1)); - redemption.push(PendingRedemption { end_block, assets }) - }); + PendingWithdrawals::::append(&caller, PendingRedemption { end_block, assets }); Self::deposit_event(Event::WithdrawalInitiated(caller, effectively_withdrawn)); - Ok(().into()) + Ok(()) } /// Attempts to complete all currently pending redemption processes @@ -640,8 +630,8 @@ pub mod pallet { /// as soon as the aforementioned conditions are met, regardless of /// whether the other `AssetWithdrawal`s in the same `PendingWithdrawal` set /// can also be closed successfully. - #[transactional] #[pallet::weight(T::WeightInfo::complete_withdraw())] + #[transactional] pub fn complete_withdraw(origin: OriginFor) -> DispatchResult { let caller = T::AdminOrigin::ensure_origin(origin.clone())?; let current_block = frame_system::Pallet::::block_number(); @@ -764,9 +754,9 @@ pub mod pallet { units: T::Balance, location: MultiLocation, amount: T::Balance, - ) -> DispatchResultWithPostInfo { + ) -> DispatchResult { if units.is_zero() { - return Ok(().into()); + return Ok(()); } let availability = AssetAvailability::Liquid(location); @@ -789,7 +779,7 @@ pub mod pallet { } Self::deposit_event(Event::AssetAdded(asset_id, units, recipient, amount)); - Ok(().into()) + Ok(()) } /// Removes liquid assets @@ -798,9 +788,9 @@ pub mod pallet { asset_id: T::AssetId, units: T::Balance, recipient: Option, - ) -> DispatchResultWithPostInfo { + ) -> DispatchResult { if units.is_zero() { - return Ok(().into()); + return Ok(()); } Self::ensure_not_native_asset(&asset_id)?; @@ -1001,7 +991,7 @@ pub mod pallet { fn do_update_index_token_locks(user: &T::AccountId) { let locks = IndexTokenLocks::::get(user); if !locks.is_empty() { - Self::do_insert_index_token_locks(user, IndexTokenLocks::::get(user)) + Self::do_insert_index_token_locks(user, locks) } } @@ -1057,7 +1047,7 @@ pub mod pallet { // native asset can't be added Self::ensure_not_native_asset(&asset_id)?; // mint asset into the treasury account - T::Currency::deposit(asset_id, &Self::treasury_account(), units)?; + T::Currency::deposit(asset_id, &Self::treasury_account(), units)?; // ??? Bryan - where is this token coming from? // mint PINT into caller's balance increasing the total issuance T::IndexToken::deposit_creating(caller, nav); Ok(()) @@ -1087,7 +1077,7 @@ pub mod pallet { let index_token = Self::saft_equivalent(saft_nav)?; // mint the given units of the SAFT asset into the treasury's account - T::Currency::deposit(asset_id, &Self::treasury_account(), units)?; + T::Currency::deposit(asset_id, &Self::treasury_account(), units)?; // ??? Bryan - what prevents this from minting other assets? e.g. DOT // mint PINT into caller's balance increasing the total issuance T::IndexToken::deposit_creating(caller, index_token); diff --git a/pallets/committee/src/lib.rs b/pallets/committee/src/lib.rs index 45d9c349ef..5cf4629e6c 100644 --- a/pallets/committee/src/lib.rs +++ b/pallets/committee/src/lib.rs @@ -110,7 +110,7 @@ pub mod pallet { /// Store a mapping (hash) -> Proposal for all existing proposals. #[pallet::storage] - pub type Proposals = StorageMap<_, Blake2_128Concat, HashFor, Proposal, OptionQuery>; + pub type Proposals = StorageMap<_, Identity, HashFor, Proposal, OptionQuery>; /// Store a mapping (hash) -> () for all proposals that have been executed #[pallet::storage] @@ -247,14 +247,11 @@ pub mod pallet { /// store Returns an error if the data type used for the nonce /// exceeds is maximum value fn take_and_increment_nonce() -> Result> { - let nonce = ProposalCount::::get(); - match nonce.checked_add(&T::ProposalNonce::one()) { - Some(next) => { - ProposalCount::::set(next); - Ok(nonce) - } - None => Err(Error::ProposalNonceExhausted), - } + ProposalCount::::try_mutate(|nonce| -> Result> { + let current = nonce.clone(); + *nonce = current.checked_add(&One::one()).ok_or(Error::ProposalNonceExhausted)?; + Ok(current) + }) } pub fn active_proposals() -> Vec> { @@ -294,25 +291,26 @@ pub mod pallet { /// `Storage: ActiveProposals (r:1 w:1) + Votes (r1) * len(proposals)` fn upkeep(n: BlockNumberFor) -> Weight { // ActiveProposals.retain (r:1 w:1) - let mut consumed = T::DbWeight::get().reads_writes(1, 1); + let mut reads: Weight = 1; + let writes: Weight = 1; // clear out proposals that are no longer active ActiveProposals::::mutate(|proposals| { // consumed weight for all `Storage: Votes (r1)` lookups - let read_vote = T::DbWeight::get().reads(1); - consumed = consumed.saturating_add(read_vote.saturating_mul(proposals.len() as Weight)); + reads = reads.saturating_add(proposals.len() as Weight); proposals.retain(|hash| if let Some(votes) = Self::get_votes_for(hash) { votes.end > n } else { false }) }); - consumed + T::DbWeight::get().reads_writes(reads, writes) } /// Used to check if an origin is signed and the signer is a member of /// the committee pub fn ensure_member(origin: OriginFor) -> Result>, DispatchError> { let who = ensure_signed(origin)?; - Ok(CommitteeMember::new(who.clone(), Members::::get(who).ok_or(Error::::NotMember)?)) + let members = Members::::get(&who).ok_or(Error::::NotMember)?; + Ok(CommitteeMember::new(who, members)) } } diff --git a/pallets/remote-asset-manager/src/lib.rs b/pallets/remote-asset-manager/src/lib.rs index 6b6722ee23..c7d262d59c 100644 --- a/pallets/remote-asset-manager/src/lib.rs +++ b/pallets/remote-asset-manager/src/lib.rs @@ -474,7 +474,7 @@ pub mod pallet { value: T::Balance, payee: RewardDestination>, ) -> DispatchResultWithPostInfo { - let _ = ensure_signed(origin.clone())?; + let _ = ensure_signed(origin.clone())?; // ??? Bryan - why ensure signed & ensure admin? T::AdminOrigin::ensure_origin(origin)?; if value.is_zero() { return Ok(().into()); @@ -497,6 +497,8 @@ pub mod pallet { let call = PalletStakingCall::::Bond(Bond { controller: controller.clone(), value, payee }); let encoder = call.encoder::(&asset); + // ??? Bryan - who is paying for the XCM execution fee on dest chain? + let xcm = Xcm::Transact { origin_type: OriginKind::SovereignAccount, require_weight_at_most: config.weights.bond, @@ -550,6 +552,8 @@ pub mod pallet { }); let encoder = call.encoder::(&asset); + // ??? Bryan - who is paying for the XCM execution fee on dest chain? also the proxy deposit + let xcm = Xcm::Transact { origin_type: OriginKind::SovereignAccount, require_weight_at_most: config.weights.add_proxy, From c7d1271b2b71591f26e609b3d7cb69b88f34a634 Mon Sep 17 00:00:00 2001 From: Bryan Chen Date: Tue, 21 Sep 2021 09:54:22 +1200 Subject: [PATCH 2/4] remove inline review comments --- pallets/asset-index/src/lib.rs | 4 ++-- pallets/remote-asset-manager/src/lib.rs | 5 ----- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/pallets/asset-index/src/lib.rs b/pallets/asset-index/src/lib.rs index 4c3f44254d..bf40386f97 100644 --- a/pallets/asset-index/src/lib.rs +++ b/pallets/asset-index/src/lib.rs @@ -1047,7 +1047,7 @@ pub mod pallet { // native asset can't be added Self::ensure_not_native_asset(&asset_id)?; // mint asset into the treasury account - T::Currency::deposit(asset_id, &Self::treasury_account(), units)?; // ??? Bryan - where is this token coming from? + T::Currency::deposit(asset_id, &Self::treasury_account(), units)?; // mint PINT into caller's balance increasing the total issuance T::IndexToken::deposit_creating(caller, nav); Ok(()) @@ -1077,7 +1077,7 @@ pub mod pallet { let index_token = Self::saft_equivalent(saft_nav)?; // mint the given units of the SAFT asset into the treasury's account - T::Currency::deposit(asset_id, &Self::treasury_account(), units)?; // ??? Bryan - what prevents this from minting other assets? e.g. DOT + T::Currency::deposit(asset_id, &Self::treasury_account(), units)?; // mint PINT into caller's balance increasing the total issuance T::IndexToken::deposit_creating(caller, index_token); diff --git a/pallets/remote-asset-manager/src/lib.rs b/pallets/remote-asset-manager/src/lib.rs index c7d262d59c..7f4638aff9 100644 --- a/pallets/remote-asset-manager/src/lib.rs +++ b/pallets/remote-asset-manager/src/lib.rs @@ -474,7 +474,6 @@ pub mod pallet { value: T::Balance, payee: RewardDestination>, ) -> DispatchResultWithPostInfo { - let _ = ensure_signed(origin.clone())?; // ??? Bryan - why ensure signed & ensure admin? T::AdminOrigin::ensure_origin(origin)?; if value.is_zero() { return Ok(().into()); @@ -497,8 +496,6 @@ pub mod pallet { let call = PalletStakingCall::::Bond(Bond { controller: controller.clone(), value, payee }); let encoder = call.encoder::(&asset); - // ??? Bryan - who is paying for the XCM execution fee on dest chain? - let xcm = Xcm::Transact { origin_type: OriginKind::SovereignAccount, require_weight_at_most: config.weights.bond, @@ -552,8 +549,6 @@ pub mod pallet { }); let encoder = call.encoder::(&asset); - // ??? Bryan - who is paying for the XCM execution fee on dest chain? also the proxy deposit - let xcm = Xcm::Transact { origin_type: OriginKind::SovereignAccount, require_weight_at_most: config.weights.add_proxy, From 4ab290401efa0ad590be49a7c3a66ee06564bdd6 Mon Sep 17 00:00:00 2001 From: Bryan Chen Date: Tue, 21 Sep 2021 09:57:53 +1200 Subject: [PATCH 3/4] improve --- pallets/asset-index/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pallets/asset-index/src/lib.rs b/pallets/asset-index/src/lib.rs index f7d2413c42..6041aa821d 100644 --- a/pallets/asset-index/src/lib.rs +++ b/pallets/asset-index/src/lib.rs @@ -757,7 +757,7 @@ pub mod pallet { units: T::Balance, amount: T::Balance, ) -> DispatchResult { - Assets::::get(&asset_id).ok_or(Error::::AssetNotExists)?; + ensure!(Assets::::contains_key(&asset_id), Error::::AssetNotExists); if units.is_zero() { return Ok(()); From d7164d24803a49d70387109f4b9c5c2e2a839776 Mon Sep 17 00:00:00 2001 From: Bryan Chen Date: Tue, 21 Sep 2021 10:49:58 +1200 Subject: [PATCH 4/4] fix --- pallets/asset-index/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pallets/asset-index/src/lib.rs b/pallets/asset-index/src/lib.rs index 6041aa821d..1971d679e8 100644 --- a/pallets/asset-index/src/lib.rs +++ b/pallets/asset-index/src/lib.rs @@ -1138,7 +1138,7 @@ pub mod pallet { units: T::Balance, location: MultiLocation, amount: T::Balance, - ) -> DispatchResultWithPostInfo { + ) -> DispatchResult { let origin = T::AdminOrigin::successful_origin(); let origin_account_id = T::AdminOrigin::ensure_origin(origin.clone()).unwrap();