feat: better handling of DID deletions

[ntn-x2]

WIP. Fixes https://github.com/KILTprotocol/ticket/issues/3673.

[ntn-x2]

@rflechtner @Ad96el just a ping to keep this in your inbox, since we need to discuss about the approach proposed here.

[Ad96el]

I’m not a huge fan of this deletion process. We generally aim to keep operations simple and atomic, allowing them to be batched together to achieve the desired state change. Introducing so much logic for something so minor doesn’t seem worth it to me. I also dnt think this design would improve the user experience.

We should avoid introducing inflationary storage maps that are unknown to the pallet. This doesn’t feel like a good Substrate practices and should maybe only considered as a last resort.

I would strongly prefer a simple hook that determines whether a DID can be deleted or not. Any error returned to the user should provide enough guidance to make it clear what steps they need to take.

Sorry for not paying closer attention to the discussion earlier - I would have raised my concerns sooner.

[ntn-x2]

@Ad96el I totally agree with you, @rflechtner was of a different opinion tho. So I think it's worth discussing about this a bit further, and then choose a strategy. I also prefer not overcomplicating the runtime and simply fail, and let the client figure out what needs to happen before, with a proper strategy that perhaps combines a good error message + a support runtime API. Let's give @rflechtner one more chance to express his opinion before we decide the way forward.

[rflechtner]

I am not opposed to just rejecting attempts to delete a DID in order to keep things simple, but let me be clear that the client will NOT solve this for the user, so it's going to be the user who's got to figure out what needs to be done. Good, expressive error messages are vital in this case, which could be a problem given that substrate errors are pretty limited - there's no more info than can be expressed in an enum variant, is there?

The only runtime api that I see as helpful would be the one providing extrinsics via a runtime api. I generally like this idea, still it gives me a bit of a headache because it seems really dangerous to sign transactions composed by external clients, and can only be really recommended if you run a node yourself.

Generally I think this discussion is exactly the of the kind of questions that we would tackle in the tech board.

[ntn-x2]

@rflechtner

Good, expressive error messages are vital in this case, which could be a problem given that substrate errors are pretty limited - there's no more info than can be expressed in an enum variant, is there?

I just went over all the FRAME pallets, and there's no single instance where what's returned as part of a pallet error is more than a simple enum variant, with no additional info. But that applies for pallet errors, meaning, we can have a runtime API check_deletion_requirements - naming to be changed - which can return expressive errors.

The only runtime api that I see as helpful would be the one providing extrinsics via a runtime api. I generally like this idea, still it gives me a bit of a headache because it seems really dangerous to sign transactions composed by external clients, and can only be really recommended if you run a node yourself.

This can be left to clients to decide what they want to do with this runtime API. I think it helps to have it there, but of course we will put a disclaimer that the result of this call still has to be checked before being taken, signed and submitted. Honestly, I don't think this is any different than the security assumptions we have today, especially since the check for the metadata Merkle root: we are relying on the full node to give us the right metadata. Nevertheless, a node could give us malicious metadata and we could be signing a transfer tx when we think we're trying to claim a w3n.

In general, I would see perhaps two different APIs: one that returns a list of "linked" elements, which is trivially an enum that we can expand on the more DID-linked stuff we add, and the second is a vec of Calls containing the delete operation for each of those elements. Clients (or users for what matter) can do as they please with the information returned, knowing that if any of those returns a non-empty collection, the did.delete call will fail.