feat: pallet assets pop api integration #71

Daanvdplas · 2024-04-09T17:23:04Z

Pop API + partial local fungibles implementation.

Size of contract pop-api/examples/fungibles/ compared to the psp22 extension in ink-examples:

Fungibles contract:
Original wasm size: 31.3K, Optimized: 7.6K
Template contract:
Original wasm size: 30.3K, Optimized: 7.0K

The Pop API design is 95% in its final state. #94 will be the last addition which refactors the chain extension functions directly to the functions that are currently to be found in pop_api/src/v0/assets/fungibles.rs (see the issue description for explanation). Then some additional refactors need to be done or looked at (see todo's below). When this is all finished, the remaining local fungibles features can be added (again see below). Finally, we want to include the new chain extension logic in an iterative manner. This is by feature gating the functionality between devnet and testnet runtime. At the moment this doesn't work because (I think) the node takes both runtimes and cannot make the separation clearly between those runtimes. Last, a contract similar to the example contract should be deployed on testnet so that we can test it while upgrading Pop Network to polkadot sdk latest version.

Remaining TODOs (in the given order):

peterwht

Looking really good! Left a few comments. Also, not directly related to this PR, but why is pallet-assets labelled as TrustBackedAssets? Makes me think this instance is for reserve transferred assets. Also seems confusing for sc devs

pop-api/examples/trust_backed_assets/lib.rs

pop-api/src/v0/assets/trust_backed.rs

primitives/src/storage_keys.rs

evilrobot-01 · 2024-04-09T23:45:09Z

Also, not directly related to this PR, but why is pallet-assets labelled as TrustBackedAssets?

https://github.com/polkadot-fellows/runtimes/blob/bb42c6b12ceab03e1c41b017f3499f778701bdbb/system-parachains/asset-hubs/asset-hub-polkadot/src/lib.rs#L280

evilrobot-01 · 2024-04-09T23:52:21Z

Thinking with devex in mind, what are your thoughts around naming it like: local_assets and foreign_assets.

Foreign assets will technically be a local definition of a foreign asset. Taking guidance from asset hubs latest instances, they also now have PoolAssets, which are also 'local' assets.

peterwht · 2024-04-09T23:59:54Z

Thinking with devex in mind, what are your thoughts around naming it like: local_assets and foreign_assets.

Foreign assets will technically be a local definition of a foreign asset. Taking guidance from asset hubs latest instances, they also now have PoolAssets, which are also 'local' assets.

I like foreign assets and local assets more IMO. In the runtime we can leave it as TrustBackedAssets, but the pop-api should have something easier to understand, like: pop_api::assets and pop_api::cross_chain::assets

runtime/testnet/src/lib.rs

pop-api/src/v0/assets/trust_backed.rs

evilrobot-01 · 2024-04-10T00:11:51Z

I like foreign assets and local assets more IMO.

Local (or native for that matter) implies that they have been created by chain governance when in reality they can be created by anyone. Trust-backed at least signals 'buyer beware'.

evilrobot-01 · 2024-04-10T00:18:03Z

I guess this essentially came about due to stablecoins on AH

evilrobot-01 · 2024-04-10T08:41:40Z

This API available to smart contracts should implement https://github.com/inkdevhub/standards/blob/master/PSPs/psp-22.md#Interfaces, including the error types.

AlexD10S · 2024-04-29T08:43:09Z

I am getting an error when building this branch using the pop-cli:
pop up parachain -f ./tests/zombienet.toml -p https://github.com/r0gue-io/pop-node#daan/feat-assets

What it does under the hood is:

cargo build --release -p pop-node

But I am getting an error:

 error[E0412]: cannot find type `Vec` in this scope
     --> /Users/alexbean/Documents/rogue/pop-node/runtime/testnet/src/extensions.rs:294:13
      |
  294 | ) -> Result<Vec<u8>, DispatchError>
      |             ^^^ not found in this scope
      |
  help: consider importing one of these items
      |
  1   + use crate::Vec;
      |
  1   + use scale_info::prelude::vec::Vec;
      |
  1   + use sp_std::vec::Vec;

It works in main, and after reviewing the PR I can't see what makes it fail.

Daanvdplas · 2024-05-05T10:07:05Z

I will first work on the Pop API before continuing the work on this PR @AlexD10S

Thanks for letting me know though :)

pop-api/src/v0/contracts.rs

# This is the 1st commit message: refactor: general # This is the commit message #2: init # This is the commit message #3: begin refactor # This is the commit message #4: refactor: error handling # This is the commit message #5: tests: add error handling tests # This is the commit message #6: WIP # This is the commit message #7: finalise error handling # This is the commit message #8: refactor: easier review

Daanvdplas · 2024-07-05T21:20:17Z

runtime/devnet/src/extensions/mod.rs

+	params: Vec<u8>,
+) -> Result<RuntimeCall, DispatchError> {
+	match pallet_index {
+		52 => {


There may be some issues with match at compilation time, but could you explore using Assets::index(); (see here)

As this is in the runtime, this can probably be changed to super::Assets::index() to obtain the index of the pallet in the runtime in a strongly typed manner. This should be the preferred approach over constants within the runtime.

Daanvdplas · 2024-07-05T21:22:09Z

runtime/devnet/src/extensions/mod.rs

+	match key {
+		TotalSupply(id) => {
+			env.charge_weight(T::DbWeight::get().reads(1_u64))?;
+			Ok(pallet_assets::Pallet::<T, TrustBackedAssetsInstance>::total_supply(id).encode())


Daanvdplas · 2024-07-05T21:25:00Z

runtime/devnet/src/extensions/mod.rs

+		});
+	}
+}
+// use enumflags2::BitFlags;


Daanvdplas · 2024-07-05T21:25:45Z

runtime/devnet/src/extensions/v0/assets.rs

+		0 => {
+			let id = <AssetId>::decode(&mut &params[..])
+				.map_err(|_| DispatchError::Other("DecodingFailed"))?;
+			Ok(TotalSupply(id))


Call immediately its function and write to buffer

evilrobot-01 · 2024-07-17T08:32:15Z

Idea for later:

* Deploy the fungibles contract to testnet. Have a thorough testing set for calling the contract. Run these tests after each upgrade to make sure the pop api / contract still works as it should.

Too late in the development cycle imo. If its already deployed to the testnet, then updating the runtime on the testnet to see if breaks things means we would have to revert the update if it did. Unless you mean using chopsticks to sync testnet state and then run tests against that. Would be nice to be able to do this purely in Rust though.

evilrobot-01

Great work! I think there is still some cleanup to do before it should be merged. There seems to be some obvious compilation issues around features too.

Note: I did not review the devnet extensions implementation as I simply ran out of steam. This PR is too big to easily digest, especially with large breaks in between. My suggestion is to consider the feedback and once ready for review again I will look at the extensions implementation in more detail.

I appreciate this was a large undertaking, but a consideration for future ones is to consider trying to break things out into smaller PRs to make it easier for the reviewer to digest.

pop-api/examples/fungibles/Cargo.toml

pop-api/examples/fungibles/lib.rs

evilrobot-01 · 2024-07-17T10:29:34Z

pop-api/src/v0/assets/mod.rs

+/// Move some assets from the sender account to another, keeping the sender account alive.
+#[inline]
+pub fn transfer_keep_alive(id: AssetId, target: AccountId, amount: Balance) -> Result<()> {
+	ChainExtensionMethod::build(u32::from_le_bytes([


Can we refactor this into a descriptively named function in lib.rs, taking the four parameters with proper names so that it is easy for others to call it and to ensure the correct values are provided. It should be easy for future API implementors to do this properly.

For example:
fn chain_extension_method(version: u8, function: u8, module: u8, dispatchable: u8)

Also happy with some abbreviated function name, provided not too difficult to decipher, as it will be used a lot and it would be good if it could fit on one line. Unlikely depending on parameter constant names tho.

We can even omit the version in a function defined within each version module:

fn chain_extension_method(function: u8, module: u8, dispatchable: u8) { super::chain_extension_method(0, function, module, dispatchable) }

Added to #94, great suggestion

Taking it a bit further, it could probably be

fn build(version: u8, function: u8, module: u8, dispatchable: u8) -> ChainExtensionMethod { ChainExtensionMethod::build(u32::from_le_bytes([version, function, module, dispatchable])) }

@evilrobot-01 @Daanvdplas PR for refracting is here: #121
Please take a look and let me know if there are any feedback

pop-api/src/v0/assets/mod.rs

runtime/devnet/src/extensions/mod.rs

peterwht

The design is quite nice, so kudos for the iterations and improvements.

I am still not sold on the implementation, in particular using integer values directly is generally not recommended, and will introduce an undue maintenance burden. However, I would like to see this PR get merged as the functionality seems sound, and then we can make further improvements and refactoring. It has also become a very large PR to review!

pop-api/integration-tests/src/local_fungibles.rs

pop-api/src/v0/nfts.rs

peterwht · 2024-07-17T13:44:37Z

runtime/devnet/src/extensions/mod.rs

+	params: Vec<u8>,
+) -> Result<RuntimeCall, DispatchError> {
+	match pallet_index {
+		52 => {


There may be some issues with match at compilation time, but could you explore using Assets::index(); (see here)

runtime/devnet/src/extensions/mod.rs

evilrobot-01 · 2024-07-18T08:35:14Z

However, I would like to see this PR get merged as the functionality seems sound, and then we can make further improvements and refactoring. It has also become a very large PR to review!

I disagree on the merge. When I looked at it there was clear compilation issues if other features were enabled in the pop_api crate. We should not be merging until at least those are resolved.

I do agree that it has become too big to review, so suggested addressing what can be addressed easily and then closing the PR (to retain as reference if required) and renaming the branch to something like api to start afresh. All remaining issues can then be tackled as PRs to that branch before a final review and merge to main.

Daanvdplas · 2024-07-18T13:57:00Z

Suggestion is to review first 3 commits with last commit next to you that has small improvements.

Notes:

Looking for a word that describes any call a contract can make using the pop api, now it is Pop API call (as a result, the error UnknownCall).
Warning for a double import of AccountId, this will be resolved with refactor (pop api): match chain extension functions to standard. #94

This will be the last changes before I close the PR and open a fresh branch where further improvements will be pushed to.

evilrobot-01

Just two final comments.

primitives/src/lib.rs

evilrobot-01 · 2024-07-19T02:23:15Z

pop-api/src/v0/assets/mod.rs

+/// Move some assets from the sender account to another, keeping the sender account alive.
+#[inline]
+pub fn transfer_keep_alive(id: AssetId, target: AccountId, amount: Balance) -> Result<()> {
+	ChainExtensionMethod::build(u32::from_le_bytes([


Taking it a bit further, it could probably be

fn build(version: u8, function: u8, module: u8, dispatchable: u8) -> ChainExtensionMethod { ChainExtensionMethod::build(u32::from_le_bytes([version, function, module, dispatchable])) }

Daanvdplas · 2024-07-26T12:10:01Z

Closing in favour of #132

Daanvdplas requested review from peterwht, evilrobot-01 and al3mart April 9, 2024 17:23

Daanvdplas force-pushed the daan/feat-assets branch from 2e8f2d9 to 3fc65ba Compare April 9, 2024 17:25

peterwht reviewed Apr 9, 2024

View reviewed changes

evilrobot-01 reviewed Apr 10, 2024

View reviewed changes

runtime/testnet/src/lib.rs Outdated Show resolved Hide resolved

pop-api/src/v0/assets/trust_backed.rs Outdated Show resolved Hide resolved

Daanvdplas marked this pull request as draft April 22, 2024 19:36

Daanvdplas added 2 commits May 19, 2024 17:38

feat: pallet assets pop api integration

7c2eac2

style: renaming assets example

418aa63

Daanvdplas force-pushed the daan/feat-assets branch 2 times, most recently from d88ee51 to 92a8465 Compare May 22, 2024 11:00

peterwht reviewed May 23, 2024

View reviewed changes

pop-api/src/v0/contracts.rs Outdated Show resolved Hide resolved

Daanvdplas removed the request for review from al3mart May 23, 2024 10:52

refactor: chain extension tests

3d385df

Daanvdplas force-pushed the daan/feat-assets branch from f1df725 to 3d385df Compare June 4, 2024 06:14

Daanvdplas added 5 commits June 5, 2024 19:16

fix: add error handling other than ModuleError

fd46a27

test: add transfer

1022926

chore: remove polkadot launch

ecc82e2

refactor: error handling

f824801

Daanvdplas force-pushed the daan/feat-assets branch from 874218f to f824801 Compare June 20, 2024 16:38

refactor: clarify todos

4670503

Daanvdplas commented Jul 5, 2024

View reviewed changes

runtime/devnet/src/extensions/mod.rs

});

}

}

// use enumflags2::BitFlags;

Copy link

Collaborator Author

Daanvdplas Jul 5, 2024

Choose a reason for hiding this comment

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

#103

Daanvdplas commented Jul 5, 2024

View reviewed changes

Daanvdplas mentioned this pull request Jul 5, 2024

feat (pop api): transfer_from and decrease_allowance #104

Closed

refactor: final bits

47e2ea6

Daanvdplas marked this pull request as ready for review July 5, 2024 22:01

fix: compiling

9dcfccb

Daanvdplas force-pushed the daan/feat-assets branch from ff9ff6d to 9dcfccb Compare July 6, 2024 11:29

Daanvdplas requested review from evilrobot-01 and peterwht July 6, 2024 11:32

fix: cannot find macro vec error

b067eda

evilrobot-01 requested changes Jul 17, 2024

View reviewed changes

Daanvdplas added 2 commits July 17, 2024 17:47

refactor: decoding failed and commenting out unimplemented tests

8730f90

style: renaming

b591222

peterwht requested changes Jul 17, 2024

View reviewed changes

Daanvdplas added 4 commits July 18, 2024 11:43

refactor: bits and pieces

c2acbb5

docs: add docs to code

3639320

fix: replace test code to api fungibles

81e9844

fix: own review + fixing CI fail

023d3dc

Daanvdplas requested review from evilrobot-01 and peterwht July 18, 2024 14:43

evilrobot-01 reviewed Jul 19, 2024

View reviewed changes

refactor: add Error variants index + clippy allowance

e683e16

chungquantin mentioned this pull request Jul 23, 2024

refactor: build chain extension method #121

Merged

Daanvdplas closed this Jul 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: pallet assets pop api integration #71

feat: pallet assets pop api integration #71

Daanvdplas commented Apr 9, 2024 •

edited

Loading

peterwht left a comment •

edited

Loading

evilrobot-01 commented Apr 9, 2024

evilrobot-01 commented Apr 9, 2024

peterwht commented Apr 9, 2024

evilrobot-01 commented Apr 10, 2024

evilrobot-01 commented Apr 10, 2024

evilrobot-01 commented Apr 10, 2024

AlexD10S commented Apr 29, 2024

Daanvdplas commented May 5, 2024

Daanvdplas Jul 5, 2024

peterwht Jul 17, 2024

evilrobot-01 Jul 17, 2024

Daanvdplas Jul 5, 2024

Daanvdplas Jul 5, 2024

Daanvdplas Jul 5, 2024

Daanvdplas Jul 5, 2024

evilrobot-01 commented Jul 17, 2024

evilrobot-01 left a comment

evilrobot-01 Jul 17, 2024

evilrobot-01 Jul 17, 2024

Daanvdplas Jul 18, 2024

evilrobot-01 Jul 19, 2024

chungquantin Jul 23, 2024

peterwht left a comment •

edited

Loading

peterwht Jul 17, 2024

evilrobot-01 commented Jul 18, 2024

Daanvdplas commented Jul 18, 2024 •

edited

Loading

evilrobot-01 left a comment

evilrobot-01 Jul 19, 2024

Daanvdplas commented Jul 26, 2024

feat: pallet assets pop api integration #71

feat: pallet assets pop api integration #71

Conversation

Daanvdplas commented Apr 9, 2024 • edited Loading

Pop API + partial local fungibles implementation.

peterwht left a comment • edited Loading

Choose a reason for hiding this comment

evilrobot-01 commented Apr 9, 2024

evilrobot-01 commented Apr 9, 2024

peterwht commented Apr 9, 2024

evilrobot-01 commented Apr 10, 2024

evilrobot-01 commented Apr 10, 2024

evilrobot-01 commented Apr 10, 2024

AlexD10S commented Apr 29, 2024

Daanvdplas commented May 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evilrobot-01 commented Jul 17, 2024

evilrobot-01 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterwht left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evilrobot-01 commented Jul 18, 2024

Daanvdplas commented Jul 18, 2024 • edited Loading

evilrobot-01 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daanvdplas commented Jul 26, 2024

Daanvdplas commented Apr 9, 2024 •

edited

Loading

peterwht left a comment •

edited

Loading

peterwht left a comment •

edited

Loading

Daanvdplas commented Jul 18, 2024 •

edited

Loading