refactor(mainnet): assets config #465

al3mart · 2025-02-05T03:37:42Z

Adds assets into mainnet runtime:

pallet_assets
pallet_nfts

Refactors ProxyType such that assets calls are taken into account.

Include pallet_assets in the runtime
Include pallet_nfts in the runtime
Review pallet_assets configuration
Review pallet_nfts configuration
Implement configuration unit tests for pallet_assets
Implement configuration unit tests for pallet_nfts
Include asset specific proxy types in mainnet runtime.

Relevant configuration:

pallet-assets

// Accounts for `Asset` max size.
pub const AssetDeposit: Balance = deposit(1, 210);
// Enough to keep the balance in state.
pub const AssetAccountDeposit: Balance = deposit(1, 16);
// Accounts for the key length + 
// Value max length without name and symbol, which are accounted per byte. 
pub const MetadataDepositBase: Balance = deposit(1, 38);

pallet-nfts

// All features enabled.
pub NftsPalletFeatures: PalletFeatures = PalletFeatures::all_enabled();
// Accounts for `AccountBalance` max size.
// Key = 68 bytes (4+16+32+16), Value = 52 bytes (4+32+16)
pub const NftsCollectionBalanceDeposit: Balance = deposit(1, 120);
// Accounts for state from `Collection` +
// `CollectionRoleOf` +
// `CollectionConfigOf` +
// `CollectionAccount`
// Refer to `ensure_collection_deposit` test for specifics.
pub const NftsCollectionDeposit: Balance = deposit(4, 294);
// Accounts for `CollectionApprovals` max size.
// Key = 116 bytes (4+16+32+16+32+16), Value = 21 bytes (1+4+16)
pub const NftsCollectionApprovalDeposit: Balance = deposit(1, 137);
// Accounts for `Item` storage item max size.
pub const NftsItemDeposit: Balance = deposit(1, 861);
// Key = max(size_of(item_metadata_key), size_of(collection_metadata_key)) + Balance: 16 bytes
pub const NftsMetadataDepositBase: Balance = deposit(1, 56);
// Accounts for key size of `Attribute`.
pub const NftsAttributeDepositBase: Balance = deposit(1, 175);

I'd like to bring the attention to the item NftsItemDeposit. The cost for this deposit is ~0.208 DOT. Considering that this is held by item. I'd suggest to lower this value to something more similar to 0.01 DOT. Lowering the barrier to access to the assets created in these collections.

I'm not that concerned about the rest of deposits myself, but happy to hear different opinions.

Relevant reference to tackle these numbers: RFC#45

[sc-2764]

codecov-commenter · 2025-02-05T04:46:00Z

Codecov Report

Attention: Patch coverage is 87.45020% with 63 lines in your changes missing coverage. Please review.

Project coverage is 76.10%. Comparing base (cae5b19) to head (6a7fb18).

Files with missing lines	Patch %	Lines
runtime/mainnet/src/apis.rs	0.00%	44 Missing ⚠️
runtime/mainnet/src/config/proxy.rs	87.65%	10 Missing ⚠️
runtime/common/src/proxy.rs	60.86%	9 Missing ⚠️

@@                         Coverage Diff                         @@
##           al3mart/refactor-mainnet-config     #465      +/-   ##
===================================================================
+ Coverage                            75.78%   76.10%   +0.31%     
===================================================================
  Files                                   83       86       +3     
  Lines                                17127    17606     +479     
  Branches                             17127    17606     +479     
===================================================================
+ Hits                                 12980    13399     +419     
- Misses                                3884     3936      +52     
- Partials                               263      271       +8

Files with missing lines	Coverage Δ
pallets/nfts/runtime-api/src/lib.rs	`0.00% <ø> (ø)`
runtime/common/src/lib.rs	`100.00% <ø> (+88.46%)`	⬆️
runtime/mainnet/src/config/assets.rs	`100.00% <100.00%> (ø)`
runtime/mainnet/src/lib.rs	`66.23% <ø> (ø)`
runtime/common/src/proxy.rs	`60.86% <60.86%> (ø)`
runtime/mainnet/src/config/proxy.rs	`88.38% <87.65%> (+5.20%)`	⬆️
runtime/mainnet/src/apis.rs	`0.00% <0.00%> (ø)`

... and 2 files with indirect coverage changes

fmt refactor(collator): not necessary import

runtime/mainnet/src/config/assets.rs

runtime/common/src/proxy.rs

al3mart · 2025-02-11T16:49:20Z

runtime/mainnet/src/config/proxy.rs

+			ProxyType::NonTransfer =>
+				!is_transfer_call(c) &&
+					// Wrapped transfer calls are filtered too.
+					matches!(
+						c,
+						RuntimeCall::Utility(pallet_utility::Call::batch { calls }) |
+						RuntimeCall::Utility(pallet_utility::Call::batch_all { calls })
+						if !calls.iter().any(|call| is_transfer_call(call))
+					) && matches!(
+					c,
+					RuntimeCall::Utility(pallet_utility::Call::as_derivative { call, .. })
+					if !is_transfer_call(call)
+				),


NonTransfer can't dispatch calls that transfer funds, nor wrap calls that transfer funds.

al3mart · 2025-02-11T16:49:44Z

runtime/mainnet/src/config/proxy.rs

+		RuntimeCall::Balances { .. } |
+			RuntimeCall::Assets { .. } |
+			RuntimeCall::Nfts { .. } |
+			RuntimeCall::Treasury { .. }


Though that Treasury calls should also be filtered.

al3mart · 2025-02-11T17:33:00Z

runtime/mainnet/src/config/assets.rs

+			let max_collection_size = pallet_nfts::Collection::<Runtime>::storage_info()
+				.first()
+				.and_then(|info| info.max_size)
+				.unwrap_or_default();
+
+			// Left for the reviewer to verify discrepancy.
+			// println!("STORAGE INFO MAX SIZE: {:?}", &max_collection_size);
+			// let key = Blake2_128Concat::max_len::<CollectionId>();
+			// let value_size = AccountId::max_encoded_len() + Balance::max_encoded_len() + (8 * 4);
+			// println!("MAX CALCULATED SIZE: {:?}", key + value_size);
+


Found a discrepancy between the result of storage_info().max_size and the result I obtained calculating the size as shown.

Curious to hear if I missed anything.

You are missing the struct CollectionDetails, you are only accounting for the generic types of this struct :)

This line supposedly accounts for those 🤔
// let value_size = AccountId::max_encoded_len() + Balance::max_encoded_len() + (8 * 4);

pub struct CollectionDetails<AccountId, DepositBalance> { /// Collection's owner. pub owner: AccountId, /// The total balance deposited by the owner for all the storage data associated with this /// collection. Used by `destroy`. pub owner_deposit: DepositBalance, /// The total number of outstanding items of this collection. pub items: u32, /// The total number of outstanding item metadata of this collection. pub item_metadatas: u32, /// The total number of outstanding item configs of this collection. pub item_configs: u32, /// The total number of attributes for this collection. pub attributes: u32, }

Daanvdplas

Apart from the internal conversation we had regarding this PR everything looks good to proceed to making this ready for a final review!

Daanvdplas · 2025-02-12T08:27:36Z

Cargo.toml

-pop-runtime-devnet = { path = "runtime/devnet", default-features = true }   # default-features=true required for `-p pop-node` builds
-pop-runtime-mainnet = { path = "runtime/mainnet", default-features = true } # default-features=true required for `-p pop-node` builds
-pop-runtime-testnet = { path = "runtime/testnet", default-features = true } # default-features=true required for `-p pop-node` builds
+pop-runtime-devnet = { path = "runtime/devnet", default-features = true }                 # default-features=true required for `-p pop-node` builds


unnecessary fmt changes

Daanvdplas · 2025-02-12T08:33:20Z

runtime/mainnet/src/config/assets.rs

+			// Accounts for the key length +
+			// Value max length without name and symbol, which are accounted per byte.


Daanvdplas · 2025-02-12T08:36:44Z

runtime/mainnet/src/config/assets.rs

+			let max_collection_size = pallet_nfts::Collection::<Runtime>::storage_info()
+				.first()
+				.and_then(|info| info.max_size)
+				.unwrap_or_default();
+
+			// Left for the reviewer to verify discrepancy.
+			// println!("STORAGE INFO MAX SIZE: {:?}", &max_collection_size);
+			// let key = Blake2_128Concat::max_len::<CollectionId>();
+			// let value_size = AccountId::max_encoded_len() + Balance::max_encoded_len() + (8 * 4);
+			// println!("MAX CALCULATED SIZE: {:?}", key + value_size);
+


You are missing the struct CollectionDetails, you are only accounting for the generic types of this struct :)

al3mart force-pushed the al3mart/refactor-mainnet-assets branch from 4327776 to 4be4a08 Compare February 5, 2025 03:46

al3mart changed the title ~~refactor(mainnet): include assets~~ refactor(mainnet): assets config Feb 5, 2025

al3mart force-pushed the al3mart/refactor-mainnet-config branch 3 times, most recently from c81b9d6 to 702b7f2 Compare February 7, 2025 10:32

al3mart force-pushed the al3mart/refactor-mainnet-assets branch 2 times, most recently from bb73e34 to b7a3e9d Compare February 10, 2025 18:37

al3mart added 3 commits February 11, 2025 15:58

refactor(mainnet): include assets

cf99ad4

fmt refactor(collator): not necessary import

test(assets): add nfts config unit tests

726b377

refactor(proxy): Add assets & smart contract proxy type

e614927

al3mart force-pushed the al3mart/refactor-mainnet-assets branch from bdbe427 to e614927 Compare February 11, 2025 14:58

al3mart added 3 commits February 11, 2025 16:42

refactor(assets): implement nfts runtime api

f8fdeea

fix(pallet-nfts-api): workspace linters are not defined

6b4b505

docs(assets): clarify deposits

5c75dc7

al3mart commented Feb 11, 2025

View reviewed changes

runtime/mainnet/src/config/assets.rs Outdated Show resolved Hide resolved

al3mart commented Feb 11, 2025

View reviewed changes

runtime/common/src/proxy.rs Outdated Show resolved Hide resolved

chore(proxy): improve usage of references

e94a545

al3mart commented Feb 11, 2025

View reviewed changes

al3mart added 3 commits February 11, 2025 17:57

docs(assets): update src reference

13a2392

chore(assets): update MetadataDepositBase

47b3c92

docs(assets): remove TODOs

e5f4212

al3mart commented Feb 11, 2025

View reviewed changes

al3mart requested review from Daanvdplas, peterwht and a team and removed request for peterwht and Daanvdplas February 11, 2025 17:34

al3mart added the ready-for-high-level-review label Feb 11, 2025

al3mart marked this pull request as ready for review February 11, 2025 17:34

al3mart removed the request for review from Daanvdplas February 11, 2025 18:04

Daanvdplas self-requested a review February 12, 2025 08:23

refactor(proxy): revert SmartContract ProxyType

6a7fb18

Daanvdplas reviewed Feb 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(mainnet): assets config #465

refactor(mainnet): assets config #465

al3mart commented Feb 5, 2025 •

edited

Loading

codecov-commenter commented Feb 5, 2025 •

edited

Loading

al3mart Feb 11, 2025 •

edited

Loading

al3mart Feb 11, 2025

al3mart Feb 11, 2025

Daanvdplas Feb 12, 2025

al3mart Feb 12, 2025

Daanvdplas left a comment

Daanvdplas Feb 12, 2025

Daanvdplas Feb 12, 2025

Daanvdplas Feb 12, 2025

		// Accounts for the key length +
		// Value max length without name and symbol, which are accounted per byte.

refactor(mainnet): assets config #465

Are you sure you want to change the base?

refactor(mainnet): assets config #465

Conversation

al3mart commented Feb 5, 2025 • edited Loading

codecov-commenter commented Feb 5, 2025 • edited Loading

Codecov Report

al3mart Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

al3mart Feb 11, 2025

Choose a reason for hiding this comment

al3mart Feb 11, 2025

Choose a reason for hiding this comment

Daanvdplas Feb 12, 2025

Choose a reason for hiding this comment

al3mart Feb 12, 2025

Choose a reason for hiding this comment

Daanvdplas left a comment

Choose a reason for hiding this comment

Daanvdplas Feb 12, 2025

Choose a reason for hiding this comment

Daanvdplas Feb 12, 2025

Choose a reason for hiding this comment

Daanvdplas Feb 12, 2025

Choose a reason for hiding this comment

al3mart commented Feb 5, 2025 •

edited

Loading

codecov-commenter commented Feb 5, 2025 •

edited

Loading

al3mart Feb 11, 2025 •

edited

Loading