review & cleanup

[xlc]

Some minor cleanup & fixes.
Add some inline comment for questions.

Some more clone can be removed by having help method taking &T::AccountId instead of T::AccountId.
I don't see any mechanism to prevent admin from minting of other non-related assets. Some refactor is likely required to prevent it by leveraging some type system.
XCM related fee handling also needs more consideration. The fee will be paid by parachain account so the pallets needs to charge someone for the fee.. Also with new introduction of BuyWeights, the current code needs update.

[CLAassistant]

All committers have signed the CLA.

[mattsse]

Thanks so much for this review!

Some more clone can be removed by having help method taking &T::AccountId instead of T::AccountId.

Definitely, will follow up on this #403

I don't see any mechanism to prevent admin from minting of other non-related assets. Some refactor is likely required to prevent it by leveraging some type system.

Yes, the initial minting process design (add_asset) left some room for improvement. We received some similar feedback and are about to make registering and minting to separate parts #383 so that add_asset will require that the asset is already registered.

XCM related fee handling also needs more consideration. The fee will be paid by parachain account so the pallets needs to charge someone for the fee..

Very good point, we still need to do our research on the substrate native xcm benchmarking suite. Since these XCM Transact calls will be executed via governance we expect them to be paid from the treasury directly.
There will likely be a fee charged by deposit in order to cover potential XCM staking-related calls. This will definitely benefit from the xcm weightinfo.
also we expect all the Transact calls to be improved significantly with the upcoming Query/Response Message types, but I guess they're scheduled for v2.

Also with new introduction of BuyWeights, the current code needs update.

We're about to tackle this with the migration to xcm::latest

I'd like to include your improvements, so if you're fine with it, I'll strip your comments and merge the PR.

[mattsse]

the AssetIndex::add_saft function will be called by SaftRegistry::add_saft pallet which is executed via governance. The asset_id will therefore be part of the proposal.

The check

Assets::<T>::try_mutate(asset_id, |maybe_available| -> DispatchResult {
		if let Some(exits) = maybe_available.replace(AssetAvailability::Saft) {
			ensure!(exits.is_saft(), Error::<T>::ExpectedSAFT);
		}
		Ok(())
})?;

ensures that the id is either nonexistent or a known non liquid SAFT id

[xlc]

the question is, via which governance? If this is installed to Karura / Acala runtime, it is not acceptable to allow anyone other than root to have permission to mint arbitrary tokens

[mattsse]

I see. The thing with SAFT is, that there are no tokens (yet). docs.polkadotindex.com/safts-within-the-pint-index
So we would need some further restrictions on this, or disable the SAFT registry pallet for the Acala integration entirely.
The latter would make more sense since AssetId here would be CurrencyId which is a fixed set with reserved tokens right?

[xlc]

One approach is do some sub currency id that is fully owned by PINT pallets. Then PINT pallets can have full privilege to this.
So we need: type AssetId: From<PintAssetId> + TryInto<PintAssetId> and

enum PintAssetId {
  PINT,
  SAFT(u32), // or whatever identifier
}

and for Acala

enum CurrencyId {
 // current variants,
  Pint(PintAssetId),
}

[mattsse]

That's an elegant solution, we'll go with that.

[mattsse]

After some feedback, we're already in the process of splitting the registering and minting (add_asset) part. #383 #387
So at this point, we expect the asset was registered before (register_asset).

[xlc]

I still don't fully understand. You cannot mint any assets other than PINT token. So there shouldn't be any deposit call.

[mattsse]

Initially, this was intended as governance-approved mint process for PINT and assets alike.
You're right this doesn't seem to be sound, since there is no guarantee that these assets we're depositing here are also available in the PINT's parachain account on the asset's native chain, right?
You'd recommend that we require that these assets were also send via xcm? so that the deposit here becomes a transfer?

[xlc]

Yes I am happy with a transfer from account controlled by pallet or the caller of a tx.

[mattsse]

since this is executed via governance proposal we expect this to be paid from the PINT treasury account.

[xlc]

but there needs to be some code to burn KSM from PINT treasury to accomplish the fee paid on relaychain.

[mattsse]

good point, this is still a missing feature that needs to be implemented.

[xlc]

I'd like to include your improvements, so if you're fine with it, I'll strip your comments and merge the PR.

Please go ahead.

[clearloop]

Fixed CI on the PR of forked repo, it should be triggered on the next commit : )

[mattsse]

amazing, thank you so much!
will take care of failing tests due to refactor to DispatchResult

[clearloop]

Thanks! @xlc

@mattsse what about packing the discussion in this pr into a new issue, there is a fixing for the tests run after this pr in #407