Use `AssetHolder` instead of `PortfolioId` by HenriqueNogara · Pull Request #1890 · PolymeshAssociation/Polymesh

HenriqueNogara · 2026-02-24T01:03:16Z

changelog

modified external API

Replace PortfolioId for `AssetHolder;

Asset Pallet:
- Events: ControllerTransfer, AssetBalanceUpdated;
- Extrinsics: issue, redeem, controller_transfer;
- New Errors: InvalidTransferSenderDidMatchesReceiverDid, UnauthorizedHolderKey, KeyNotFoundForDid, InsufficientTokensLocked;
Identity Pallet:
- New Errors: IdentityNotFoundForAccountPortfolio;
NFT Pallet:
- Events: NFTHoldingsUpdated
- Storage: Remove NFTOwner;
- Extrinsics: issue_nft, redeem_nft, controller_transfer;
- New Errors: NFTIsNotLocked;
Settlement Pallet:
- Events: InstructionAffirmed, AffirmationWithdrawn, InstructionAutomaticallyAffirmed;
- Constants: replace MaxNumberOfPortfolios for MaxNumberOfAssetHolders;
- Storage: AffirmsReceived, UserAffirmations;
- Extrinsics: affirm_with_receipts, execute_manual_instruction, add_and_affirm_instruction, affirm_instruction, withdraw_affirmation, reject_instruction, affirm_with_receipts_with_count, affirm_instruction_with_count, reject_instruction_with_count, withdraw_affirmation_with_count, add_and_affirm_with_mediators,
Runtime Api
- asset: transfer_report;
- nft: transfer_report;
- settlement: get_affirmation_count

tgntr · 2026-03-05T12:26:13Z

pallets/nft/src/lib.rs

-                return weight;
            }
-
            Weight::zero()


should we count reads/writes and return actual weight here? migrate_to_v7 also returns zero weight

tgntr · 2026-03-05T12:29:50Z

pallets/settlement/src/lib.rs

+            polymesh_primitives::storage_migrate_on!(StorageVersion::<T>, 4, {
+                migrations::migrate_to_v4::<T>();
+            });
+            Weight::zero()


should we return the actual weight here? migrate_to_v4 actually counts reads/writes but we just return zero

tgntr · 2026-03-05T12:31:59Z

pallets/asset/src/lib.rs

        )?;

-        let updated_balance = BalanceOf::<T>::get(asset_id, portfolio.did) - value;
+        let updated_did_balance = BalanceOf::<T>::get(asset_id, holder_did) - value;


should we use checked_sub for substraction here?

If the code checks the available balance before this, then we can just use saturating_* instead of checked_* ops.

tgntr · 2026-03-05T12:34:36Z

pallets/asset/src/lib.rs

+            &[(issuer_did, current_issuer_balance)],
        )?;

        let new_issuer_balance = current_issuer_balance + amount_to_issue;


should we avoid unchecked operations and use checked_add instead?

tgntr · 2026-03-05T12:37:44Z

rpc/runtime-api/src/asset.rs

 pub type Error = Vec<u8>;

 sp_api::decl_runtime_apis! {
    #[api_version(4)]


should we bump api_version because of the changes?

tgntr · 2026-03-05T12:38:07Z

rpc/runtime-api/src/nft.rs


 sp_api::decl_runtime_apis! {

    #[api_version(2)]


should we bump api_version because of the changes?

tgntr · 2026-03-05T12:38:24Z

rpc/runtime-api/src/settlement.rs

+use polymesh_primitives::AssetHolder;

 sp_api::decl_runtime_apis! {
    #[api_version(2)]


should we bump api_version because of the changes?

tgntr · 2026-03-05T12:40:05Z

pallets/nft/src/migrations.rs

+        None,
+    );
+
+    if let Some(cursor) = result.maybe_cursor {


should we instead loop until maybe_cursor returns None?

Neopallium

First round of review.

Neopallium · 2026-03-04T12:53:16Z

.circleci/config.yml

      - run:
          name: Upgrade chain to current version.
-          command: cargo run --locked --bin tools -- upgrade_chain ../assets/ci-runtime/polymesh_runtime_develop.compact.compressed.wasm
+          command: cargo run --features current_release --locked --bin tools -- upgrade_chain ../assets/ci-runtime/polymesh_runtime_develop.compact.compressed.wasm


Since this needs to run/compile against the old chain, we need to use previous_release here.

Neopallium · 2026-03-04T13:02:18Z

rpc/runtime-api/src/asset.rs

-            sender_portfolio: PortfolioId,
-            receiver_portfolio: PortfolioId,
+            sender: AssetHolder,
+            receiver: AssetHolder,


Need to bump the version of this Runtime API method.

Neopallium · 2026-03-04T13:02:36Z

rpc/runtime-api/src/nft.rs

        fn transfer_report(
-            sender_portfolio: PortfolioId,
-            receiver_portfolio: PortfolioId,
+            sender: AssetHolder,
+            receiver: AssetHolder,


Same here. Bump the version. And the other Runtime API methods below.

Neopallium · 2026-03-04T13:06:11Z

rpc/src/asset.rs

    fn transfer_report(
        &self,
-        sender_portfolio: PortfolioId,
-        receiver_portfolio: PortfolioId,
+        sender: AssetHolder,
+        receiver: AssetHolder,
        asset_id: AssetId,
        transfer_value: Balance,
        skip_locked_check: bool,
        at: Option<<Block as BlockT>::Hash>,
    ) -> RpcResult<Vec<DispatchError>> {


I would like to start dropping these RPC endpoints, since it is much better for clients to directly call the Runtime APIs instead. Otherwise this RPC impl. needs to be updated to translate the new AssetHolder type to the old Runtime API call which only supports PortfolioId.

Neopallium · 2026-03-05T13:25:35Z

pallets/asset/src/lib.rs

+        let primary_key = pallet_identity::Pallet::<T>::get_primary_key(primary_did)
+            .ok_or(Error::<T>::KeyNotFoundForDid)?;
+
+        ensure!(primary_key == account_id, Error::<T>::UnauthorizedHolderKey);


If secondary_key is None the caller should always be the primary key. We could avoid looking up the DID's primary key by checking the caller account.

Neopallium · 2026-03-05T13:28:55Z

pallets/asset/src/lib.rs

+    /// - The transfer is between different DIDs;
+    /// - If [`AssetHolder::Portfolio`], it must exist;
+    /// - The sender has sufficient balance for the transfer (taking into account locks).
+    pub fn ensure_valid_holdings(


We might have to allow transfers (settlements) when the sender/receiver are the same DID. Otherwise this is going to trip up integrator. If the sender/receiver DIDs are the same, we can skip the more expensive compliance checks and stats updates.

HenriqueNogara changed the title ~~New portfolio~~ New Portfolio Type Feb 24, 2026

HenriqueNogara requested review from Neopallium and tgntr February 24, 2026 11:31

Neopallium force-pushed the develop_v8 branch from f2a0cad to d2e3192 Compare February 27, 2026 13:20

HenriqueNogara added 7 commits February 28, 2026 13:21

Use AssetHolder insteas of PortfolioId - (identity, portfolio, asset)

2284285

Use AssetHolder instead of PortfolioId - (nft)

431e5f3

Use AssetHolder instead of PortfolioId - (settlement)

3f3efc6

Use AssetHolder instead of PortfolioId - (sto + ca + rpc)

87048a8

Fix benchmarks

20a9a64

Fix tests

a9fd462

Fix benchmarks

2b9cb61

HenriqueNogara force-pushed the new-portfolio branch from d4fb284 to 2b9cb61 Compare March 2, 2026 01:46

HenriqueNogara changed the title ~~New Portfolio Type~~ Use AssetHolder instead of PortfolioId Mar 2, 2026

HenriqueNogara added 8 commits March 2, 2026 11:41

Update rust-integration tests

a9219ff

Add feature flag when upgrading to new chain

53b09c6

Fix integration tests

d825af7

Fix coverage scrpit

424977b

Update schema

a53ddcc

Add migrations for settlements and NFTs

2d3e16e

Fix NFT migration

48697f7

Fix NFT migration

6f160a5

tgntr reviewed Mar 5, 2026

View reviewed changes

Neopallium reviewed Mar 5, 2026

View reviewed changes

Conversation

HenriqueNogara commented Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

changelog

modified external API

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neopallium left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

HenriqueNogara commented Feb 24, 2026 •

edited

Loading