Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: flag to disallow asset management changes #875

Draft
wants to merge 3 commits into
base: bonded-coins-audit-fixes
Choose a base branch
from

Conversation

rflechtner
Copy link
Contributor

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
Copy link
Contributor Author

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

@rflechtner rflechtner requested review from ntn-x2 and abdulmth March 11, 2025 16:36
@ntn-x2
Copy link
Member

ntn-x2 commented Mar 11, 2025

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
Copy link
Contributor Author

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
Copy link
Member

ntn-x2 commented Mar 12, 2025

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
Copy link

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants