Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pallet transfers allowlist #1251

Merged
merged 85 commits into from
Apr 11, 2023

Conversation

thea-leake
Copy link
Contributor

@thea-leake thea-leake commented Mar 13, 2023

Description

This adds a pallet to handle allowlist for transfers.

It allows users to add an allowance for an account, and if allowances present, then checks against the allowance for the sending account/currrency will be limited to the recievers for which allowances are defined, and allow/blocked at block range.

Fixes #1144

Changes and Descriptions

Adds Transfer Allowlist pallet

Allowlist pallet, and tests added.

Type of change

  • [ x] New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • Pallet unit tests with externalities provided mock env

Checklist:

  • [x ] I have added Rust doc comments to structs, enums, traits and functions
  • [x ] I have made corresponding changes to the documentation
  • [ x] I have performed a self-review of my code
  • [ x] My changes generate no new warnings
  • [ x] I have added tests that prove my fix is effective or that my feature works
  • [x ] New and existing unit tests pass locally with my changes
  • [ x] I rebased on the latest main branch

Copy link
Collaborator

@mustermeiszer mustermeiszer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks already neat!

@thea-leake thea-leake changed the title WIP Pallet transfers allowlist Pallet transfers allowlist Mar 20, 2023
@thea-leake thea-leake marked this pull request as ready for review March 20, 2023 20:45
Copy link
Contributor

@lemunozm lemunozm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very neat @thea-leake and very good coverage!

I left a lot of comments, but most of them are minor suggestions & questions. Good job!

pallets/transfer-allowlist/Cargo.toml Show resolved Hide resolved
pallets/transfer-allowlist/src/lib.rs Outdated Show resolved Hide resolved
pallets/transfer-allowlist/src/lib.rs Outdated Show resolved Hide resolved
pallets/transfer-allowlist/src/lib.rs Outdated Show resolved Hide resolved
pallets/transfer-allowlist/src/lib.rs Outdated Show resolved Hide resolved
pallets/transfer-allowlist/src/lib.rs Outdated Show resolved Hide resolved
pallets/transfer-allowlist/src/lib.rs Outdated Show resolved Hide resolved
match <AccountCurrencyTransferAllowance<T>>::get((&account_id, &currency_id, &receiver))
{
Some(_) => {
T::ReserveCurrency::unreserve(&account_id, T::Deposit::get());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mustermeiszer @mikiquantum should we use pallet-fees here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pallet fees do not support reserve/unreserve, afaik, only one way transfer/burn/etc

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But should T::Deposit::get() come from a key? At least we do so for Anchors::pre_commit().

Maybe reserve/unreserve actions need to be added to pallet-fees. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes @lemunozm I would definitely define a key in the pallet fees and read the amount from it, that even if for now we do not support the reserve/unreserve logic yet there. That way the UIs can know up front operational costs.
@thea-leake ^^

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WIP updating pallet to use fees/feekey. I've got the pallet set up for using fees pallet(config, mocks etc), and additional fee key added. Switching out deposit/reserve logic to use feekey in progress.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Pallet is switched over to use fee key & getting fee val with fees pallet. Also have mock set up to set key val for allowance creation with fees pallet to ensure fees pulled w/ correct key -- FeeKey used in pallet itself is still loosely coupled in allowance pallet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a comment regarding the mock part @thea-leake.

Regarding reserve()/unreserve(), @mikiquantum I've created an issue to add them to pallet-fees: #1282

pallets/transfer-allowlist/src/lib.rs Show resolved Hide resolved
pallets/transfer-allowlist/src/tests.rs Show resolved Hide resolved
Copy link
Contributor

@mikiquantum mikiquantum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just trying to be curious. 😄 From my limited understanding, it looks good.

libs/types/src/domain_address.rs Show resolved Hide resolved
XCM(H256),
/// DomainAddress sending location from connectors
Address(DomainAddress),
/// Etherium address, for cases where we would have a standalone Eth address
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Etherium address, for cases where we would have a standalone Eth address
/// Ethereum address, for cases where we would have a standalone Eth address

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 fixed

libs/types/src/locations.rs Outdated Show resolved Hide resolved

impl From<MultiLocation> for Location {
fn from(ml: MultiLocation) -> Self {
// using hash here as mulitlocation is significantly larger than any other enum type here
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// using hash here as mulitlocation is significantly larger than any other enum type here
// using hash here as multilocation is significantly larger than any other enum type here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I, for one, am in favor of having a MulletLocation 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 fixed


impl From<VersionedMultiLocation> for Location {
fn from(vml: VersionedMultiLocation) -> Self {
// using hash here as mulitlocation is significantly larger than any other enum type here
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// using hash here as mulitlocation is significantly larger than any other enum type here
// using hash here as multilocation is significantly larger than any other enum type here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 fixed


/// Storage item for allowances specified for a sending account, currency type and recieving location
#[pallet::storage]
#[pallet::getter(fn sender_currency_reciever_allowance)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#[pallet::getter(fn sender_currency_reciever_allowance)]
#[pallet::getter(fn sender_currency_receiver_allowance)]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 fixed, also changed to match storage.

OptionQuery,
>;

/// Storage item for allowances specified for a sending account, currency type and recieving location
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Storage item for allowances specified for a sending account, currency type and recieving location
/// Storage item for allowances specified for a sending account, currency type and receiving location

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 fixed

);
Ok(())
}
Some(_) => Err(DispatchError::Other("Impossible allowance count")),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In which scenario would this branch be possible?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just had a comment in the same line. This case doesn't belong to the model and can never happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've combined the storage items for delay and count as per @mikiquantum's suggestion, which has removed that case as a possibility with the types so that branch is no longer there. :)
(was not possible to reach before, however types would have accommodated that branch)

pallets/transfer-allowlist/src/lib.rs Show resolved Hide resolved
/// This allows us to keep storage map vals to known/bounded sizes.
#[pallet::storage]
#[pallet::getter(fn sender_currency_restriction_set)]
pub type AccountCurrencyTransferRestriction<T: Config> = StorageDoubleMap<
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldnt be possible to merge this storage with AccountCurrencyDelay and have a tuple value (u64, BlockNumber). There should be an storage reduction based on the storage prefix+key.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Merged storage items

Copy link
Contributor

@lemunozm lemunozm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a very clean review feedback, thanks for all your detailed answer to my question/suggestions @thea-leake

pallets/transfer-allowlist/src/lib.rs Outdated Show resolved Hide resolved
libs/types/src/domain_address.rs Show resolved Hide resolved
Comment on lines 75 to 82
pub type DepositBalanceOf<T> = <<T as Config>::ReserveCurrency as Currency<
<T as frame_system::Config>::AccountId,
>>::Balance;

//
// Storage
//
pub type AllowanceDetailsOf<T> = AllowanceDetails<<T as frame_system::Config>::BlockNumber>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI We usually place these type aliases at the top, before pub trait Config: frame_system::Config {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 fixed

pub blocked_at: BlockNumber,
}

impl<BlockNumber> AllowanceDetails<BlockNumber>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should make this actually implement the Default trait

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 fixed

/// An operation expecting one or more allowances for a sending Account/Currency set, where none present
NoAllowancesSet,
/// Attempted to create allowance for existing Sending Account, Currency, and Receiver combination
ConflictingAllowanceSet,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the /// comment, I guess this means that this allowance already exists? If so, let's make it consistent with similar error names:

Suggested change
ConflictingAllowanceSet,
DuplicateAllowance,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 fixed

AllowanceCountArithmeticError,
/// No matching allowance for Location/Currency
NoMatchingAllowance,
/// No matching delay
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you expand on this error doc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 expanded

Comment on lines 285 to 364
let blocked_at = match Self::sender_currency_delay(&account_id, currency_id) {
Some(delay) => <frame_system::Pallet<T>>::block_number().saturating_add(delay),
_ => <frame_system::Pallet<T>>::block_number(),
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be a bit easier to read with a .map().unwrap_or() pattern. Up to you 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the merging of the delay and count storage items, there's another option to check/destructure, so I've kept the match, but I agree with how it was before .map().unwrap_or() would be clearer. :)

pallets/transfer-allowlist/src/lib.rs Show resolved Hide resolved
pallet-balances = { git = "https://github.com/paritytech/substrate", default-features = true, branch = "polkadot-v0.9.32" }
pallet-fees = { path = "../fees" }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You do not need to add pallet-fees/pallet-treasury/pallet-authorship to test this. You can do it much more manageably with the mock in libs/mocks/src/fees.rs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, thanks! Yeah that would be a lot more convenient :) I wanted to verify the reserve/unreserve amounts, and verify the pallet was using the FeeKey correctly and ensure that the reserve/unreserve logic was all being handled correctly in here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine that 👍🏻. Please, if in the process you feel like something in the mock should be changed/improved, share it!

Copy link
Collaborator

@mustermeiszer mustermeiszer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really, really good, clean and nice!

But if I understood the code correctly we have 3 logical problems that must be addressed (if I got it right)

  • add_or_update_allowance_delay MUST only be active after the previously set delay has passed
  • remove_allowance_delay MUST only be active after the previously set delay has passed
  • purge_transfer_allowance MUST only be possible of the blocked_at has already passed!

Otherwise, the protection this pallet provides only serves for honest but wrongly inputed receiving addresses. Not for protecting against malicious users who got access to a key.

If the first is enough for us, then we can remove the concept of a delay.

cc: @offerijns for clarification if protection is needed.

Comment on lines 30 to 31
/// Test
TestLocal(u64),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we hide this behind a feature flag? and maybe have a fixed codec index of 255 in order to net mess up tests that rely on decoding?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I've currently got it set to the codec index of 255, and set to only std so as to not be compiled for Wasm. Though if we want to feature gate it a different way I'm ok with that too :).

libs/types/src/locations.rs Outdated Show resolved Hide resolved
libs/types/src/locations.rs Show resolved Hide resolved
pallets/transfer-allowlist/src/lib.rs Show resolved Hide resolved
Comment on lines 184 to 185
/// CatchAll for Allowance Count arithmetic errors -- largely for coverage for errors that should be impossible
AllowanceCountArithmeticError,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use enum ArithmeticError here instead. It impls Into<DIspatchError>.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 using ArithmeticErrorwith ensure ops :)

pallets/transfer-allowlist/src/lib.rs Show resolved Hide resolved
Comment on lines 341 to 422
match <AccountCurrencyTransferAllowance<T>>::get((&account_id, &currency_id, &receiver))
{
Some(_) => {
T::ReserveCurrency::unreserve(
&account_id,
T::Fees::fee_value(T::AllowanceFeeKey::get()),
);
<AccountCurrencyTransferAllowance<T>>::remove((
&account_id,
&currency_id,
&receiver,
));
Self::decrement_or_remove_allowance_count(&account_id, &currency_id)?;
Self::deposit_event(Event::TransferAllowancePurged {
sender_account_id: account_id,
currency_id,
receiver,
});
Ok(())
}
None => Err(DispatchError::from(Error::<T>::NoMatchingAllowance)),
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should ONLY be possible of the blocked_at has already passed!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 updated to ensure blocked_at has already passed.

Comment on lines 375 to 388
let (count, _) =
Self::get_account_currency_restriction_count_delay(&account_id, &currency_id)
.unwrap_or((0, Some(0u32.into())));
<AccountCurrencyTransferCountDelay<T>>::insert(
&account_id,
&currency_id,
(count, Some(delay)),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let (count, _) =
Self::get_account_currency_restriction_count_delay(&account_id, &currency_id)
.unwrap_or((0, Some(0u32.into())));
<AccountCurrencyTransferCountDelay<T>>::insert(
&account_id,
&currency_id,
(count, Some(delay)),
let (count, delay) =
Self::get_account_currency_restriction_count_delay(&account_id, &currency_id)
.unwrap_or((0, Some(delay)));
<AccountCurrencyTransferCountDelay<T>>::insert(
&account_id,
&currency_id,
(count, Some(delay)),

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, not sure if actually advantage ^^

#[pallet::weight(10_000 + T::DbWeight::get().reads_writes(0, 1).ref_time())]
/// Adds an account/currency delay
/// Calling on an existing combination will update the existing delay value
pub fn add_or_update_allowance_delay(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem here is that

  • attacker can set delay to zero
  • add arbitrary off-board address
  • send tokens

all in one block and batch.

#[pallet::call_index(4)]
#[pallet::weight(10_000 + T::DbWeight::get().reads_writes(0, 1).ref_time())]
/// Removes an existing sending account/currency delay
pub fn remove_allowance_delay(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same goes as above.

Comment on lines 154 to 194
#[pallet::storage]
#[pallet::getter(fn get_account_currency_restriction_count_delay)]
pub type AccountCurrencyTransferCountDelay<T: Config> = StorageDoubleMap<
_,
Twox64Concat,
T::AccountId,
Twox64Concat,
T::CurrencyId,
(u64, Option<T::BlockNumber>),
OptionQuery,
>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#[pallet::storage]
#[pallet::getter(fn get_account_currency_restriction_count_delay)]
pub type AccountCurrencyTransferCountDelay<T: Config> = StorageDoubleMap<
_,
Twox64Concat,
T::AccountId,
Twox64Concat,
T::CurrencyId,
(u64, Option<T::BlockNumber>),
OptionQuery,
>;
struct Delay<BlockNumber> {
current: BlockNumber,
valid_until: Option<BlockNumber>,
}
#[pallet::storage]
#[pallet::getter(fn get_account_currency_restriction_count_delay)]
pub type AccountCurrencyTransferCountDelay<T: Config> = StorageDoubleMap<
_,
Twox64Concat,
T::AccountId,
Twox64Concat,
T::CurrencyId,
(u64, Option<Delay<T::BlockNumber>>),
OptionQuery,
>;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we use this, and have the following extrinsics

  • add_delay

    • if AccountCurrencyTransferCountDelay is (count, None) → simply add new delay of kind Delay {current: delay, next: None}
  • update_delay

    • if AccountCurrencyTransferCountDelay is (count, Some(delay) → only add if the delay.valid_until has passed
  • purge_delay → is possible to remove storage if count is zero and valid_until has passed

  • rm_delay → only works

    • if AccountCurrencyTransferCountDelay is (count, Some(delay) → and valid_until is None and then sets valid_until as now + delay.current

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently have the pallet updated to use new delay and allow delay purge or modification after current delay has passed--with a set future modifiable extrinsic exposed with the functionality of rm_delay. Looking into refactoring the storage at the moment to clean up the readability of the matching/destructuring (due to the nested options etc).

Comment on lines +115 to +159
// pallet fees takes a treasury impl as assoc type
impl pallet_treasury::Config for Runtime {
type ApproveOrigin = EnsureSignedBy<Admin, u64>;
type Burn = ();
type BurnDestination = ();
type Currency = Balances;
type MaxApprovals = ();
type OnSlash = Treasury;
type PalletId = TreasuryPalletId;
type ProposalBond = ();
type ProposalBondMaximum = ();
type ProposalBondMinimum = ();
type RejectOrigin = EnsureSignedBy<Admin, u64>;
type RuntimeEvent = RuntimeEvent;
type SpendFunds = ();
type SpendOrigin = EnsureSignedBy<Admin, u64>;
type SpendPeriod = ();
type WeightInfo = ();
}

parameter_types! {
pub const DefaultFeeValue: Balance = 1;
}

// pallet fees depends on authorship being configured for runtime.
// Tight coupling--no assoc type for fees
impl pallet_authorship::Config for Runtime {
type EventHandler = ();
type FilterUncle = ();
type FindAuthor = ();
type UncleGenerations = ();
}

// used to set/retrieve reserve fee amount
// so we can surface this to the frontend
// actual reserve/unreserve handled by reserve currency type
impl pallet_fees::Config for Runtime {
type Currency = Balances;
type DefaultFeeValue = DefaultFeeValue;
type FeeChangeOrigin = EitherOfDiverse<EnsureRoot<Self::AccountId>, EnsureSignedBy<Admin, u64>>;
type FeeKey = FeeKey;
type RuntimeEvent = RuntimeEvent;
type Treasury = Treasury;
type WeightInfo = ();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would go with mock fees to simplify this before merging it 🤓

All this could be simplified to:

impl pallet_mock_fees::Config for Runtime {
	type Balance = Balance;
	type FeeKey = u8;
}

You can check pallet-anchors or pallet-nft as an example of how to use it in the tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah having the mocks would definitely be cleaner, I think it would be best to switch to the mocks after benchmarking is done so that the weight/time from those dependencies are taken into account (though in this case it shouldn't really affect the benchmarks much if at all), then switch to use the mocks so the tests aren't coupled with the other pallet dependencies/changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be best to switch to the mocks after benchmarking is done so that the weight/time from those dependencies are taken into account

Mocks will run only under test. The real benchmarks that generates the weights will run without test and will use the correct implementations coming from the runtime. So there is no issue with benchmark times using trait mocks.

Anyway, no hurry!

pallets/transfer-allowlist/src/lib.rs Outdated Show resolved Hide resolved
pallets/transfer-allowlist/src/lib.rs Show resolved Hide resolved
Copy link
Collaborator

@mustermeiszer mustermeiszer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If clauses are checking the wrong way for times. Otherwise changes look good!

Comment on lines 57 to 67

#[pallet::pallet]
#[pallet::generate_store(pub(super) trait Store)]
pub struct Pallet<T>(_);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No requirement. But might be good to get started with using this always.

Suggested change
#[pallet::pallet]
#[pallet::generate_store(pub(super) trait Store)]
pub struct Pallet<T>(_);
/// The current storage version.
const STORAGE_VERSION: StorageVersion = StorageVersion::new(0);
#[pallet::pallet]
#[pallet::generate_store(pub(super) trait Store)]
#[pallet::storage_version(STORAGE_VERSION)]
pub struct Pallet<T>(_);


#[pallet::call_index(4)]
#[pallet::weight(10_000 + T::DbWeight::get().reads_writes(0, 1).ref_time())]
/// Updates an allowance delay, only callable is the delay has been set to allow future modifications and
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Updates an allowance delay, only callable is the delay has been set to allow future modifications and
/// Updates an allowance delay, only callable if the delay has been set to allow future modifications and

pallets/transfer-allowlist/src/lib.rs Show resolved Hide resolved
Some(AllowanceMetadata {
modifiable_at: Some(modifiable_at),
..
}) if modifiable_at > current_block => Err(DispatchError::from(Error::<T>::DelayUnmodifiable)),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must be

if current_block < modifiable_at

Comment on lines 508 to 564
pub fn set_allowance_delay_future_modifiable(
origin: OriginFor<T>,
currency_id: T::CurrencyId,
) -> DispatchResult {
let account_id = ensure_signed(origin)?;
let current_block = <frame_system::Pallet<T>>::block_number();
match Self::get_account_currency_restriction_count_delay(&account_id, &currency_id) {
None => Err(DispatchError::from(Error::<T>::NoMatchingDelay)),
Some(AllowanceMetadata {
current_delay: None,
..
}) => Err(DispatchError::from(Error::<T>::NoMatchingDelay)),
Some(AllowanceMetadata {
modifiable_at: Some(modifiable_at),
..
}) if modifiable_at > current_block => Err(DispatchError::from(Error::<T>::DelayUnmodifiable)),
Some(
metadata @ AllowanceMetadata {
current_delay: Some(current_delay),
..
},
) => {
let modifiable_at = current_block.ensure_add(current_delay)?;
<AccountCurrencyTransferCountDelay<T>>::insert(
&account_id,
&currency_id,
AllowanceMetadata {
modifiable_at: Some(modifiable_at),
..metadata
},
);
Self::deposit_event(Event::TransferAllowanceDelayFutureModifiable {
sender_account_id: account_id,
currency_id,
modifiable_at,
});
Ok(())
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change as talked about (see slack for requirements)

allowance_count: 0,
modifiable_at: Some(modifiable_at),
..
}) if modifiable_at <= current_block => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must be

if current_block < modifiable_at

mustermeiszer
mustermeiszer previously approved these changes Apr 4, 2023
Copy link
Collaborator

@mustermeiszer mustermeiszer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks really nice! Thanks for closing so quickly. I would say lets merge it.

Copy link
Collaborator

@mustermeiszer mustermeiszer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approve

@thea-leake thea-leake merged commit 6665505 into centrifuge:main Apr 11, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D0-ready Pull request can be merged without special precaution and notification.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pallet-Transfer-Allowlist
6 participants