feat: flag to disallow asset management changes

[rflechtner]

re/ KILTProtocol/ticket#3800

Addresses potential centralisation concerns around manager permissions. Having this additional flag could make the extent of centralisation of powers in a pool more transparent. If it is set to false the reset_team() transaction cannot be used on this pool, in which case the manager has much more limited privileges (locking/unlocking and initiating refunding). This flag can only be set on pool creation and not changed.

Checklist:

• I have verified that the code works
  • No panics! (checked arithmetic ops, no indexing array[3] use get(3), ...)
• I have verified that the code is easy to understand
  • If not, I have left a well-balanced amount of inline comments
• I have left the code in a better state
• I have documented the changes (where applicable)
  • Either PR or Ticket to update the Docs
  • Link the PR/Ticket here

[rflechtner]

@ntn-x2 & @abdulmth what do you think about this idea? Unnecessary or helpful?

[ntn-x2]

I think it makes sense, but there start to be too many bool for the different permissions. I would encapsulate these in an (extensible) struct that includes the different permissions needed, similar to what we do in the delegation pallet. If we adopt that approach, I am fine with having two and potentially more permission types in the future.

[rflechtner]

I think it makes sense, but there start to be too many bool for the different permissions. I would encapsulate these in an (extensible) struct that includes the different permissions needed

I was thinking the same thing. The delegation pallet uses bitflags, which has terrible client-side handling, but a regular struct containing booleans should work just as well? If you think this should be 'extensible' in the sense that we reserve additional bits for future use without migrations, we could probably add dummy fields to it. Alternatively we could use bitflags on the storage level but structs in extrinsics and runtime calls.

[ntn-x2]

Yes either use bitflags in storage and provide a From impl from the input param, or use directly the input param in storage. New fields for booleans are false by default, so that is probably to keep in mind when planning for extensibility, on how to name fields. But it sounds good to me.

[abdulmth]

can't say much about the implementation details, but I think the idea is good, having the option to disable the manager from having complete control over the tokens in all accounts can help.

[rflechtner]

Yes either use bitflags in storage and provide a From impl from the input param, or use directly the input param in storage. New fields for booleans are false by default, so that is probably to keep in mind when planning for extensibility, on how to name fields. But it sounds good to me.

I experimented with nesting all immutable settings relating to bonded currencies in a struct, what do you think about that? 7c3c127

[ntn-x2]

Yes this looks already better, and harder to misuse because of the named properties.

[rflechtner]

@ntn-x2 I wasn't sure about this one, but we need to bump the storage version if we change the pool struct right?

[ntn-x2]

Yes, correct.

[rflechtner]

@ntn-x2 I fixed tests and formatting, so if you agree that we can implement migrations for peregrine separately this is ready to be reviewed. My idea is that I'd implement migrations with the help of Adel once he's back next week, as we'd need this merged by Wednesday before the audit deadline runs out.

[ntn-x2]

Yeah it looks good 👍

[ntn-x2]

Perhaps you can remove the storage version here, and introduce it in the PR that also adds the migration.

[rflechtner]

Perhaps you can remove the storage version here, and introduce it in the PR that also adds the migration.

I kinda like that the failing test reminds us of the migration we still need to do though, and it doesn't seem to mess with anything else