Auto-incremental CollectionId

[Szegoo]

This PR solves the auto-incremental collections ids task from #11783

cumulus companion: paritytech/cumulus#1460

polkadot address: 126X27SbhrV19mBFawys3ovkyBS87SGfYwtwa8J2FjHrtbmA

[Szegoo]

@jsidorenko Could you please review this?

[shawntabrizi]

The general stuff in this PR is correct, but we need something to handle the fact that Statemine and Statemint already have various IDs at different values taken.

I think a clean solution would be:

• an extrinsic which is "try_increment_id" which checks if the next id already exists, and then increments the next id
• keeping the check that ensures the same id isnt used again when creating a new asset

let me know if this makes sense.

[Szegoo]

The general stuff in this PR is correct, but we need something to handle the fact that Statemine and Statemint already have various IDs at different values taken.

I think a clean solution would be:

• an extrinsic which is "try_increment_id" which checks if the next id already exists, and then increments the next id
• keeping the check that ensures the same id isnt used again when creating a new asset

let me know if this makes sense.

Ok, we could probably make a try_increment_id and we increment the id until we find one that is not being used. Could that work?

[shawntabrizi]

@Szegoo we should avoid loops in runtime code actually, so better it only increments once, and we call it multiple times.

This is a weight / runtime safety consideration :)

[Szegoo]

@shawntabrizi I have added try_increment_id, and this function is not used to generate the id for the next collection, but for its own. Because of this the id now starts from 1 instead of 0, so there are new changes in the tests file because of this. Also, it is important to increment the id before checking if the id is used, because this way we could get stuck, cause we could never get over that id.

[shawntabrizi]

@Szegoo i think you misunderstood the solution here.

there should be a completely new extrinsic which does the try_increment_id. This can be called by a user to get the pallet out of a deadlock state where a user would not be able to create a new asset with the next id, because it may already exist.

Existing logic should just make sure that the next id to be used is not already in use. If it is in use, then the whole function fails, and the user must call try_increment_id before trying to create the next asset.

[shawntabrizi]

Here is some pseudo code:

next_id: u32;

#[call]
fn create_asset() {
    // Make sure that the asset does not already exist before we try to create it
    // If this check fails, the whole call fails, and the user must manually increment the next_id
    ensure(!asset(next_id).exists());
    do_create_asset();
    next_id ++;
}

#[call]
fn try_increment_id() {
    // Check the next id is ALREADY used. This means we are allowed to increment the id.
    // If this check fails, then the next id is already available, and they should use `create_asset`
    ensure(asset(next_id).exists());
    next_id ++;
}

[Szegoo]

@shawntabrizi I have fixed my code, I had a different idea so I did this in a totally different way. Now it is fixed, I am not done here, but I wanted you to check if this is what you wanted. I will add more tests, and I will run a benchmark to get the weight for the try_increment_id extrinsic.

[shawntabrizi]

/tip medium

[substrate-tip-bot]

@shawntabrizi A medium tip was successfully submitted for Szegoo (126X27SbhrV19mBFawys3ovkyBS87SGfYwtwa8J2FjHrtbmA on polkadot).

https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Frpc.polkadot.io#/treasury/tips

[shawntabrizi]

bot merge

[paritytech-processbot]

Error: "Check reviews" status is not passing for paritytech/cumulus#1460

[Szegoo]

/tip medium

It seems like the tip bot is getting the tip :P (https://www.dotreasury.com/dot/tips)

[shawntabrizi]

bot merge

[paritytech-processbot]

Error: "Check reviews" status is not passing for paritytech/cumulus#1460

[Szegoo]

@shawntabrizi Should be able to merge now, the "Check reviews" is passing on the companion.

[shawntabrizi]

@Szegoo thanks for the report, should be fixed now

[shawntabrizi]

/tip medium

[substrate-tip-bot]

@shawntabrizi A medium tip was successfully submitted for Szegoo (126X27SbhrV19mBFawys3ovkyBS87SGfYwtwa8J2FjHrtbmA on polkadot).

https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Frpc.polkadot.io#/treasury/tips

[shawntabrizi]

bot merge

[KiChjang]

@jsidorenko @Szegoo This trait bound here is breaking XCMv3, as we're using a MultiLocation for the CollectionId, which isn't an unsigned integer (but contains variants that can accept unsigned ints).

[Szegoo]

@KiChjang Hmm, I will look more into this, but I am not even sure if it would be possible to have this auto-increment feature since as you said the MultiLocation is used for CollectionId, and not all variants store a uint.

[KiChjang]

In that case, we should revert this change and really think about how exactly we should implement it. As it is, it's breaking code that was originally working without this PR.

[ggwpez]

Looks like it is ordered, we could let the caller increment it off-chain.
PS: Not really, since the caller could set it to the highest value...

[Szegoo]

Maybe we could for now make this feature optional for some chains that don't support XCM and don't use MultiLocation for theCollectionId.

[Szegoo]

I will continue working on this in #12057

[jsidorenko]

Super! And pls point into js/uniques-v2-main-branch afterwards

[Szegoo]

Just to make sure I understand this correctly, we should add an Incrementable trait bound to the CollectionId as I suggested earlier, so this means that we are not going to store this internal ref_id field, right?

[jsidorenko]

This is how I understood it as well. @KiChjang Pls correct us if we're wrong

[KiChjang]

That's correct, the idea is to let runtime configuration figure out what the next ID is, instead of having it become a concern for the uniques pallet itself.