paritytech / polkadot-sdk

The Parity Polkadot Blockchain SDK
https://polkadot.network/
1.78k stars 638 forks source link

Iterating Treasury and Bounty Pallets: Proposed Updates #5500

Open muharem opened 2 weeks ago

muharem commented 2 weeks ago

Summary

This document outlines the necessary changes to the Treasury and Child-/Bounty pallets to enable bounties funded by assets other than the native asset. These changes also serve as a first step toward transitioning this functionality to the Asset Hub. Additionally, it highlights some desired changes that are reasonable to implement in the current iteration. This document aims to align stakeholders and provide a place for proposing additional changes before completing the implementation.

Objectives

The next version of the Treasury and Child-/Bounty pallets aims to achieve several key objectives:

Requirements:

Solution

The Treasury pallets will be set up on the Asset Hub, initially enabling only bounty functionality. Bounty and curator approvals will be possible only through referendums on the Relay Chain. The Treasury pallet’s account ID will be made configurable and set on the Asset Hub to the account ID that already belongs to the Treasury pallet on the Relay Chain (e.g., for Polkadot, the account ID of xcm::Location(1, PalletInstance(19))). Additionally, to avoid future breaking changes and accommodate future needs, the spend call will be set up according to the RFC provided here.

In the initial phase, these pallets will serve only bounties. However, once referendums can be conducted on the Asset Hub, they can serve as the main Treasury.

The Bounty pallet will introduce two new calls:

The existing approve_bounty call and the new approve_bounty_with_curator call will transfer the requested funds to the bounty account immediately, without waiting for the next spend period, and will move the bounty status directly to Funded, skipping the Approved status. Since approval requires a referendum to pass, including confirmation and enactment periods, I don’t see a need for an additional spend period, though this can be open for discussion if there are other considerations.

The Bounty type will be extended with a parameterized AssetKind field, which can be set to the unit type on the Relay Chain to maintain backward compatibility and avoid breaking changes for clients and storage migration.

To implement isolated child bounty indexes per parent bounty, we will add a new storage item that tracks the child bounty count per parent bounty without decrementing it upon child bounty completion. We will need to adjust the keys for the ChildBountyDescriptions and ChildrenCuratorFees storage items, changing them from (ChildIndex) to (ParentIndex, ChildIndex). The ChildBounties storage item will remain unchanged.

The simplest way to support bounties for collectives that host referendums on the Collectives chain is to set up Child-/Bounty pallet instances on the Asset Hub for each collective, configuring them to recognize the collective origins from the Collectives chain. This approach would result in two additional pallet instances per collective on the Asset Hub.

Alternatively, a single set of Bounty and Treasury pallets could serve multiple collectives by introducing the unit parameter (e.g., Treasury, or Unit {Treasury, Fellowship, ..}) to the bounty and treasury spends. Backward compatibility for calls and storage items on-chain can be maintained by setting the unit type to a zero-sized type (e.g., the () unit type) for existing pallet instances that serve a single body.

Some might argue that bounty pallets should be deployed on the Collectives chain alongside the collectives' referendums and treasury pallets. While this is feasible, it introduces complexity. Since the Asset Hub manages the treasury assets, all transfers to fund, reward, or close bounties would be asynchronous, commanded from the Collectives chain and executed on the Asset Hub. This would require substantial changes to the bounty status machine. However, I'm open to considering this approach if there are compelling arguments or benefits that make it worthwhile.

It seems reasonable to implement the unit parameters at least for the bounties, allowing us to serve bounty programs with a single pallet instance for multiple bodies on the Asset Hub.

With these changes, we can achieve the objectives outlined above.

xlc commented 2 weeks ago

Best to link to previous issues, and maybe close some of them if needed https://github.com/paritytech/polkadot-sdk/issues/1141 https://github.com/paritytech/polkadot-sdk/issues/591 https://github.com/paritytech/polkadot-sdk/issues/4947 https://github.com/paritytech/polkadot-sdk/issues/5033

xlc commented 2 weeks ago

Since the Asset Hub manages the treasury assets, all transfers to fund, reward, or close bounties would be asynchronous, commanded from the Collectives chain and executed on the Asset Hub. This would require substantial changes to the bounty status machine.

I am not sure if we need to change bounty state machine because the spending methods become async? I guess it could try to track the status of each async action (ongoing/success/failure) but maybe we could just assume it should never fail because invariants?

The fellowship salary pallet is also on collectives.

With such big change, maybe it is easier to just develop a new bounty pallet alongside with the current one? so we don't need to worry about compatibility and migration at all. If you really want to avoid duplicated code, we could abstract some of the logic into a common crate.

For Spending period, I think the original intention is that the treasury can still schedule spends even if there are not enough funds in treasury for such spend (someone need to confirm that). But maybe we shouldn't schedule overspend proposals in the first place, so yeah I think we can get rid of it.

muharem commented 2 weeks ago

It can fail for example if there is no enough assets. Actions like approve_bounty_with_curator will need to pass before the pallet knows the bounty was funded. We will need some call to check the status of the payment. The bounty with a curator assigned can move back to status like 'not funded'. The state machine will need to change quite a bit. Also there are around 8 transfers that would be executed async (on approve; on child-/bounty close, cancel, two times on reward). This is all feasible.

I think we need some reasons/goal to go for more complex solution, it would be more complex migration for clients as well. Also I am not against creating a new pallet. But I wanna question reasons/goals for it as well.

I do wanna see the treasury pallets moving to the dedicated chain eventually, but from the current state and objectives I see, the proposed solution looks more reasonable to me. Please challenge.

xlc commented 2 weeks ago

I agree we need to think more about the trade-offs for different alternatives.

We need a good mechanism to control funds at remote chain regardless and we already have some pallets doing it. If it is too hard to integrate such functionality, I will say we need to do some other improvements so that it is easy to integrate with. It is too complicated is not a good reason to not do it knowing that we will need to deal many crosschain integrations in the future. It is not something can be completely avoided.

For the state machine change, it is a solved problem in many other frameworks. e.g. we can do something like

enum AsyncActionState<OnError> {
    Pending,
    Completed,
    Error,
}

enum BountyStatus {
    Proposed,
    Approved,
    Funded(AsyncActionState<OnErrorTo<Self:Approved>>),
}

And if really needed, some generic code can be used to allow the state machine to handle both sync transfer and async transfer.

The client code will need a lot of changes to support this so that's why I suggest add a new pallet. So this multiasset bounty pallet is a new feature that co-existing with the old bounty pallet. Then no migration are required. The dApps just need to support the new feature whenever they have the chance.

muharem commented 1 week ago

@xlc I agree with the motivation, we need a good tooling for cross chain scenarios.

With bounties we either setup the bounty pallets on Asset Hub (1) or on Collective (2) and in both cases we need to deal with cross-chain integrations.

  1. Collective commands permissioned operations from Collecitves chain (bounty.approve, bounty.approve_curator);

  2. Approves happens locally, transfers commanded from Collectives chain and executed on Asset Hub;

We will need to split the bounty and payment status machine. We still wanna let a bounty and curator be approved with a single referenda. Also we have multiple payments on different statuses (fund bounty; fund child bounty; reward bounty curator AND implementor (same for child bounty), on cancel - transfer money back to the treasury (same for child but to parent bounty)).

The curators and implementors will pay transaction fess on Collectives chain.


type Payments = StorageMap<Index, Payment<>, ...>

enum PaymentStatus {
    Pending,
    Completed,
    Error,
}

enum BountyStatus {
    Proposed,
    Approved,
    Funded,
    CuratorAssigned,
    ...
}

@xlc can you share your thoughts on new pallet? do you see it as extension or a standalone pallet? if second would the clients really have a choice? would not they have to adapt to a new pallet? do you have other features in mind to be implemented in a new pallet? the initial proposal breaks only keys for two storage items in child bounty pallet, but we can even avoid this if really needed.

xlc commented 1 week ago

https://github.com/paritytech/polkadot-sdk/issues/5033 is going to redesign the bounty feature and there is no chance it is a backward compatible way. So it is going to be a new pallet built from scratch with maybe some code reused from existing pallet. However given the feature requirements are still not clearly defined, I am not sure if there is anything else to discuss other than defining the requirements.

muharem commented 1 week ago

@xlc I think I covered all requests from #5533 in the proposal above with minimum breaking changes (two storage items changes the keys).

llvee commented 1 week ago

Hello, I noticed my previous comment was deleted. I am not sure what the specific reason was. If this is agitating or against some policy, please tag me in another discussion explaining more if your team has time.

I was inquiring about whether the bounties are open to anyone interested in contributing, what the best url for access is?

If there is a better channel for communication around questions like these, which one is that?

I don't currently have phone/ID access, am behind a firewall that blocks the main Parity site.

I have experience with full stack engineering, product development & research across multiple chains, ecosystems.

Some past areas where I have worked on relevant projects include: RE, Trading, Supply Chain Management, Gaming, Social Communications, New Programming Languages, Developer Tooling, Governance, International Communications

I can attempt to locate, share any relevant links that still remain.

There have also been some successful companies that have created products similar to products I was building independently.

xlc commented 1 week ago

@muharem Yeah but the initial list from Joe is not comprehensive and not covering things like delay and deposit. And with multi assets, the fees also need some consideration. AFAIK, many current bounties are not using % fees, instead, they use sub-bounty to pay for curator fees, which is fine. And with % fees, it can be a bit annoying with multiple assets and the current curator deposit is based of fees and that will be lot more annoying.

so I still demand someone to write a high level design and ensuring it covers everything, and make sure the community / bounty curators are happy with it.

muharem commented 1 week ago

@xlc I also talked with Otar. But I will double-check to make sure we aren’t missing anything. If a call is needed, I will organize one. I’ll keep you updated and invite you to the call if we have one. Thank you for your input.