rmrk-team / rmrk-ink

RMRK ink! contract

RMRK ink!

Implementation of RMRK protocol in ink! Smart contract language

Quick start

1. Make sure you have the latest cargo contract

2. Clone the repository

git clone https://github.com/rmrk-team/rmrk-ink.git

3. Compile & Build

cd ./rmrk-ink/contracts/rmrk
cargo +nightly contract build --release

3. Run ink! unit tests

cargo test

4. Integration test Start local test node. Recommended swanky-node version 1.2 or higher. Download binary or run compiled version

./swanky-node --dev --tmp

To run tests:

yarn
yarn compile
yarn test

rmrk-ink's Issues

Unit test for each crate

At the moment only the minting crate has unit tests. IMO it would be good if we could have unit tests for the rest of the crates the same way the rmrk-substrate repo had unit tests for each pallet while also having integration tests.

There are tests inside examples. 🤦

Replace asset implementation

implement replacing assets.
in method

• fn add_asset_to_token()

in events

• _emit_asset_added_to_token_event()

Limit ever-growing Vectors

Use limits and check these limits in the related functions for all ever-growing structures. Mappings are not considered.
Some of the data structures using ever-growing Vectors are:

Base::part_ids
Base::equippable_address
Part::equippable
NestingData::pending_children
NestingData::accepted_children

Acceptance criteria

• explore expected real life limits and make comments in the code
• unit test updated
• integration test updated

Instantiate Contract metadata

Why do we need metadata when instantiating the remark smart contract?
What is a base collection?
To create a NFT Marketplace am I right with the following:

Base -> Mint -> Equip -> Market

maxSupply: possibility to mint unlimited number of NFTs

In rmrk-pallets and original kusama spec maxSupply 0 equals to infinite number of tokens allowed to mint.

Add assets to multiple tokens with single call

I am using a script for RMRK collection building process. The script mints all tokens to a single account and at one point it is calls addAssetToToken, for each token in the collection. I think there should be a way to have a single call to add assets to multiple tokens, because otherwise building a RMRK collection can take a very long time.
E.g. in my case I need to build 20000 tokens collection. If I batch addAssetToToken calls and execute batches, it will took me more than 16 hours to build the collection. Currently, it is possible to add max. 4 addAssetToToken calls to a batch because of weight (proofSize) limitation.

"yarn test" fails

I am running a local node using the command "substrate-contracts-node".
On another terminal, I change dir to root directory of "rmrk-ink" and ran "yarn test", but it all the tests fails.

yarn run v1.22.19
$ mocha --require ts-node/register --recursive ./tests --extension ".spec.ts" --exit --timeout 20000


  RMRK Base tests
2023-02-02 16:18:02        API/INIT: RPC methods not decorated: transaction_unstable_submitAndWatch, transaction_unstable_unwatch
    1) Setup Base

  RMRK Merged Equippable
2023-02-02 16:18:03        API/INIT: RPC methods not decorated: transaction_unstable_submitAndWatch, transaction_unstable_unwatch
Setting up Base
    2) Merged Equippable user journey

  RMRK Multi Asset tests
2023-02-02 16:18:03        API/INIT: RPC methods not decorated: transaction_unstable_submitAndWatch, transaction_unstable_unwatch
    3) Init two rmrk contracts works
2023-02-02 16:18:04        API/INIT: RPC methods not decorated: transaction_unstable_submitAndWatch, transaction_unstable_unwatch
    4) Add assets to token and approve them

  RMRK Nesting tests
2023-02-02 16:18:05        API/INIT: RPC methods not decorated: transaction_unstable_submitAndWatch, transaction_unstable_unwatch
    5) Init two rmrk contracts works
2023-02-02 16:18:05        API/INIT: RPC methods not decorated: transaction_unstable_submitAndWatch, transaction_unstable_unwatch
    6) Add child (different user), approval works
2023-02-02 16:18:06        API/INIT: RPC methods not decorated: transaction_unstable_submitAndWatch, transaction_unstable_unwatch
    7) Add child (different user), reject works
2023-02-02 16:18:06        API/INIT: RPC methods not decorated: transaction_unstable_submitAndWatch, transaction_unstable_unwatch
    8) Add child (same user) works
2023-02-02 16:18:07        API/INIT: RPC methods not decorated: transaction_unstable_submitAndWatch, transaction_unstable_unwatch
    9) Add two parents, move/transfer child works

  Minting rmrk as psp34 tests
2023-02-02 16:18:07        API/INIT: RPC methods not decorated: transaction_unstable_submitAndWatch, transaction_unstable_unwatch
    10) create collection works
2023-02-02 16:18:08        API/INIT: RPC methods not decorated: transaction_unstable_submitAndWatch, transaction_unstable_unwatch
    11) mintNext works
2023-02-02 16:18:08        API/INIT: RPC methods not decorated: transaction_unstable_submitAndWatch, transaction_unstable_unwatch
    12) mint 5 tokens works
2023-02-02 16:18:08        API/INIT: RPC methods not decorated: transaction_unstable_submitAndWatch, transaction_unstable_unwatch
    13) token transfer works
2023-02-02 16:18:09        API/INIT: RPC methods not decorated: transaction_unstable_submitAndWatch, transaction_unstable_unwatch
    14) token aproval works
2023-02-02 16:18:09        API/INIT: RPC methods not decorated: transaction_unstable_submitAndWatch, transaction_unstable_unwatch
    15) Minting token without funds should fail


  0 passing (8s)
  15 failing

  1) RMRK Base tests
       Setup Base:
     Error: the object {
  "issue": "OUTPUT_IS_NULL"
} was thrown, throw an Error :)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

  2) RMRK Merged Equippable
       Merged Equippable user journey:
     Error: the object {
  "issue": "OUTPUT_IS_NULL"
} was thrown, throw an Error :)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

  3) RMRK Multi Asset tests
       Init two rmrk contracts works:
     Error: the object {
  "issue": "OUTPUT_IS_NULL"
} was thrown, throw an Error :)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

  4) RMRK Multi Asset tests
       Add assets to token and approve them:
     Error: the object {
  "issue": "OUTPUT_IS_NULL"
} was thrown, throw an Error :)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

  5) RMRK Nesting tests
       Init two rmrk contracts works:
     Error: the object {
  "issue": "OUTPUT_IS_NULL"
} was thrown, throw an Error :)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

  6) RMRK Nesting tests
       Add child (different user), approval works:
     Error: the object {
  "issue": "OUTPUT_IS_NULL"
} was thrown, throw an Error :)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

  7) RMRK Nesting tests
       Add child (different user), reject works:
     Error: the object {
  "issue": "OUTPUT_IS_NULL"
} was thrown, throw an Error :)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

  8) RMRK Nesting tests
       Add child (same user) works:
     Error: the object {
  "issue": "OUTPUT_IS_NULL"
} was thrown, throw an Error :)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

  9) RMRK Nesting tests
       Add two parents, move/transfer child works:
     Error: the object {
  "issue": "OUTPUT_IS_NULL"
} was thrown, throw an Error :)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

  10) Minting rmrk as psp34 tests
       create collection works:
     Error: the object {
  "issue": "OUTPUT_IS_NULL"
} was thrown, throw an Error :)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

  11) Minting rmrk as psp34 tests
       mintNext works:
     Error: the object {
  "issue": "OUTPUT_IS_NULL"
} was thrown, throw an Error :)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

  12) Minting rmrk as psp34 tests
       mint 5 tokens works:
     Error: the object {
  "issue": "OUTPUT_IS_NULL"
} was thrown, throw an Error :)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

  13) Minting rmrk as psp34 tests
       token transfer works:
     Error: the object {
  "issue": "OUTPUT_IS_NULL"
} was thrown, throw an Error :)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

  14) Minting rmrk as psp34 tests
       token aproval works:
     Error: the object {
  "issue": "OUTPUT_IS_NULL"
} was thrown, throw an Error :)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

  15) Minting rmrk as psp34 tests
       Minting token without funds should fail:
     Error: the object {
  "issue": "OUTPUT_IS_NULL"
} was thrown, throw an Error :)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

Implement Catalog as an external contract

Create separate Catalog (previously called Base) contract to implement many-to-many relation, as in evm implementation for RMRKCatalog

Rename Base to Catalog

As per recent EVM EIP changes Base was renamed to Catalog.
Adjust for consistency between implementations.

Separate the query trait

@boyswan pointed out that in ink! 4.0 self-referencing seems to no longer work. Since the existing Query trait relies on this functionality it is atm broken, meaning that calling any of the query functions will result in an error.

To fix this we will separate the Query trait into multiple traits where we will be directly referencing storage.

We will categorize the queries per each crate storage.
As an example for the MultiAsset crate, we will do something like the following:

trait MultiAssetQuery: Storage<MultiAssetData> + Storage<psp34::Data<enumerable::Balances>> {
    #[ink(message)]
    fn get_asset(&self, collection_id: AccountId, asset_id: AssetId) -> Option<Asset> {
        ...
    }

    #[ink(message)]
    fn get_assets(&self, collection_id: AccountId, asset_ids: Vec<AssetId>) -> Vec<Asset> {
        ...
    }
}

For more complex queries like the get_token which relies on multiple crates, we will make separate traits:

trait TokenQuery: QueryMinting + QueryMultiAsset + QueryNesting {
 #[ink(message)]
    fn get_token(&self, collection_id: AccountId, id_u64: u64) -> Token { ... }
}

So in essence for more complex queries we will create separate traits while the simpler ones will be implemented in one of the crate traits.

Implement cross contract check to validate equipping child

Before equipping an asset ensure that token_can_be_equipped_with_asset_into_slot
This will break part of unitests for equip.

• Make cross contract call in equip() function to call ensure_token_can_be_equipped_with_asset_into_slot()
• Fix part of the unit test that will be broken with this PR
• Introduce missing e2e test to cover equip. Perhaps new file tests/equip.spec.ts

We don't need to make it possible for an already existing PSP34 to be equipped. It would open doors to non sense equippings. If we can assume that child is always an implementer of equippable, then we can check safely.

rmrk collection id

collection_id in rmrk constructor, Is it from UtilsData::collection_id? When it change? I found last_token_id increment while minting token.

Use BeforeEach in test/ instead of setup()

Currently all integration tests call setup() function to prepare the environment for the tests.
Use BeforeEach instead of setup()
all tests are in ./tests/ folder

Over-guarded minting methods

Currently we set access control on minting trait methods by default, protecting methods from unauthorized access.

#[modifiers(only_role(CONTRIBUTOR))]
default fn mint(&mut self, to: AccountId, token_id: Id) -> Result<()> {
    self._check_amount(1)?;
    self._mint_to(to, token_id)?;
    Ok(())
}

However this means the guarded methods cannot be used from within a user-facing method of the contract, for example:

// ...Rmrk contract

#[ink(message, payable)]
pub fn public_mint_example(&mut self) -> Result<()> {
    // minting logic
    Minting::mint(self, Self::env().caller());
}

In this case, mint can only be called by a contributor. The only way around this is to call it from another contract which has been granted the contributor role.

Some possible solutions are:

• Guards are not provided, and is up to the user to implement by overriding methods
• Move all logic from the minting traits into Internal and making Internal trait public. Allows user flexibility but feels like a bit of a leak.

NftInfo Type

I noticed that there is a RMRK Type for NftInfo in "rmrk-substrate" but no NftInfo Type for "rmrk-ink".
How do I go about creating a NftInfo type for my smart contract? I need it to be able to change the owner, pricing, etc. of the NFT.

Royalty should be set in basis points

Update royalty from u8 to u16 to represent basis points ( so client side could interpret two decimal fractional royalties ie 12.25% )

Fix compile warning for allocate_packed()

Compiling rmrk v0.4.0 (/Users/mario/p/remarkink/logics)
warning: function cannot return without recursing
  --> logics/impls/rmrk/types.rs:94:5
   |
94 |     fn allocate_packed(&mut self, at: &Key) {
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot return without recursing
95 |         PackedAllocate::allocate_packed(&mut *self, at)
   |         ----------------------------------------------- recursive call site
   |
   = note: `#[warn(unconditional_recursion)]` on by default
   = help: a `loop` may express intention better if this is on purpose

Differents between Asset vs ChildNft

In add_asset_to_token function what does asset represents? Is it same like ChildNft? add_child function add_child to parent_token_id and add_asset_to_token function add_asset to token_id. Is token_id in add_asset same as parent_token_id in add_child?

Reduce arguments to 7 in new()

cargo clippy
    Checking rmrk_contract v0.4.0 (/Users/mario/p/remarkink/contracts/rmrk)
warning: this function has too many arguments (8/7)
   --> contracts/rmrk/lib.rs:200:9
    |
200 | /         /// Instantiate new RMRK contract
201 | |         #[ink(constructor)]
202 | |         pub fn new(
203 | |             name: String,
...   |
226 | |             })
227 | |         }
    | |_________^
    |
    = note: `#[warn(clippy::too_many_arguments)]` on by default
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_many_arguments

Unable to mint "asset ready" tokens in one call

Currently the only way to mint a token which has an accepted asset is through multiple methods. This is due to the fact only the token owner can add an asset without it being in pending state. This is problematic especially when minting via cross-contract calls.

The current workaround:

let to: AccountId = ...
let id = 1;

Minting::mint(self, id, self.env().caller())?;
MultiAsset::add_asset_to_token(self, id, asset_id, None)?;
PSP34::transfer(self, to, id, Default::default())?;

Will edit with possible solutions

Missing catalog address when fetching equippable data

When calling get_asset_and_equippable_data I believe catalog address is missing from the result, so parts can be fetched from the catalog when rendering a NFT.

Below is the current result from the method.

Missing ownerOfChild

Implement a function which returns parent_token_id for a child nft
get_owner_of_child(child: ChildNft) -> Result<Id, PSP34Error>

Equip Events are not emitted

Ideas and improvements around rmrk-ink

I've been starting to experiment with rmrk-ink usage from another smart contract, and have had some initial thoughts around the current implementation as well as some rough ideas on how we can improve the library.

It would be great to have a discussion as to whether any of these are worth looking further into.

---

• Decouple underlying PSP34 implementation from contract logic

Currently the lib is heavily tied to the Openbrush PSP34 implementation, but due to the volatility in establishing NFT standards and norms in dotsama, flexibility in providing different underlying implementations could be very useful. For example we could choose to support uniques-v2 (chain-ext), pallet_assets (if theres an equivalent for psp34?), rmrk-pallets (chain-ext) or anything new as alternative backends.

I don't think this is a difficult change to make, but it will require some generalisation of a few areas. I've got a very rough idea of how this might look in this branch. Areas that need further generalising are errors (everything is a PSP34 error), which may actually justify the need for more flexible error handling . Also need to consider how to select the implementation (ie. cargo flags etc).

---

• Move from Ownable to AccessControl ownership model

In its current state, the lib has the notion of a single owner. This is not flexible enough if we want the ability to compose multiple 'logic' smart contracts (or even users), and grant them minting rights. The EVM implementation also has the notion of contributor for their access control, which is something we could move towards. This should be a fairly straightforward improvement by utilising the Openbrush AccessControl lib.

---

• Distinction between "Ready to use" and "General use" implementations

Currently the logic provides both mint-to-caller and only-owner-mint which cater to different use-cases (self-minting vs controlled-minting), but not necessarily both at the same time. For example if I wanted a RMRK collection to be solely controlled by another ink! contract (ie. a game with extra logic), the mint-to-caller method is problematic. In the EVM implementation, "Lazy Minting" is an optional feature. Perhaps @steven2308 can provide some clarity on this design in the EVM lib incase I have misunderstood? My understanding is based off this explanation.

---

Edit: added other findings to the list for us to discuss.

Add catalog for assets

add catalog Mapping of asset ID to corresponding catalog address.
Implement in multiasset.rs

• get_asset_and_equippable_data()
• add_asset_entry()

Mapping<AssetId, AccountId>

Unify Child events with EVM

Since #36 was closed I am looking forward to unify the event names as well as event fields with EMV.

Streamline usage of ensure_exists()

There are 3 implementation of the same function ensure_exists()

nesting::ensure_exists
multiassets::ensure_exists()
utils::Internal::_token_exists()

Make ensure_exists() accessible for all traits from single place. Use implementation for ensure_exists() which returns the owner.

Create CI for contract

This is what a CI github action should cover.

Basic checks

• cargo check
• cargo test
• cargo clippy
• cargo fmt --all

Integration test

• run integration test on a node (yarn test)