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

Optimistic Project Funding #5162

Open
wants to merge 190 commits into
base: master
Choose a base branch
from
Open

Optimistic Project Funding #5162

wants to merge 190 commits into from

Conversation

ndkazu
Copy link
Contributor

@ndkazu ndkazu commented Jul 27, 2024

Description

This PR is related to this issue .
Through the introduction of the OPF pallet and the DISTRIBUTION pallet, we are handling the Optimistic Project Funding.
It allows users to nominate projects (whitelisted in OpenGov) with their DOT. This mechanism will be funded with a constant stream of DOT taken directly from inflation and distributed to projects based on the proportion of DOT that has nominated them. The nominations are handled by the OPF Pallet, while the project rewards distribution is handled by the Distribution Pallet.
The Distribution Pallet receives the list of Whitelisted/Nominated Projects with their respective calculated rewards. For each project, it will create a corresponding Spend that will be stored until the project reward can be claimed. At the moment, the reward claim period start corresponds to: [beginning of an Epoch_Block + BufferPeriod] (The BufferPeriod can be configured in the runtime).

User’s conviction has not been implemented in this first pallet iteration.

Integration

Review Notes

The voting round timeline is described below:

|----------Voting_Round_0-----------|----------Voting_Round_1-----------|
|---users_votes----|--funds_locked--|---users_votes----|--funds_locked--|
|------------------|--Distribution--|------------------|--Distribution--|

Terminology

The constants available in the runtime for the OPF Pallet:

  • MaxWhitelistedProjects: Maximum number of Whitelisted projects that can be handled by the pallet.
  • VoteLockingPeriod: Period during which voting is disabled.
  • VotingPeriod: Period during which voting is enabled.
  • TemporaryRewards: For test purposes only ⇒ used as a substitute for the inflation portion used for the rewards.

The constants available in the runtime for the Distribution Pallet:

  • PotId: Pot containing the funds used to pay the rewards.
  • BufferPeriod: minimum required buffer time period between project nomination and reward claim.

Checklist

pallet-distribution

  • Pallet Config
  • Helper functions
  • Extrinsics
  • Events & Tests
  • Benchmarking & weights
  • Remove Dev-mode
  • Proper documentation

pallet-opf

  • Pallet Config
  • Helper functions
  • Extrinsics
  • Events & Tests
  • Benchmarking & weights
  • Remove Dev-mode
  • Proper documentation

@ndkazu ndkazu requested a review from a team as a code owner July 27, 2024 03:25
@ndkazu ndkazu marked this pull request as draft July 27, 2024 08:06
Copy link
Contributor

@marcuspang marcuspang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind writing briefly how the voting would work? I feel this is quite a big deviation from what I had in mind

substrate/frame/distribution/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/distribution/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/distribution/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/distribution/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/distribution/src/types.rs Outdated Show resolved Hide resolved
substrate/frame/distribution/src/functions.rs Outdated Show resolved Hide resolved
substrate/frame/distribution/src/types.rs Outdated Show resolved Hide resolved
substrate/frame/distribution/src/lib.rs Outdated Show resolved Hide resolved
umbrella/Cargo.toml Outdated Show resolved Hide resolved
substrate/frame/distribution/src/functions.rs Show resolved Hide resolved
@ndkazu
Copy link
Contributor Author

ndkazu commented Jul 30, 2024

Do you mind writing briefly how the voting would work? I feel this is quite a big deviation from what I had in mind

The Voting Logic would be managed in a different pallet (pallet_vote for example).
The main difference is that the result of the voting process would be a whitelisted project of type ProjectInfo (which is defined in pallet_distribution). Then, through coupling of pallet_vote & pallet_distribution (this would be done in the Pallet::Config of pallet_vote), pallet_vote could populate the storage Projects found in pallet_distribution.

@lolmcshizz
Copy link

Do you mind writing briefly how the voting would work? I feel this is quite a big deviation from what I had in mind

The Voting Logic would be managed in a different pallet (pallet_vote for example). The main difference is that the result of the voting process would be a whitelisted project of type ProjectInfo (which is defined in pallet_distribution). Then, through coupling of pallet_vote & pallet_distribution (this would be done in the Pallet::Config of pallet_vote), pallet_vote could populate the storage Projects found in pallet_distribution.

Just to be clear here - "whitelisting" a project is done via OpenGov and creates a list of "projects" (addresses) that can be funded via OPF.

"Voting" as part of OPF implementation is users locking their DOT at some conviction to allocate funding to projects that are on the whitelist - this is not done through OpenGov and should be handled through the implementation of OPF.

Is this what you are implementing?

let voter_holds = BalanceOf::<T>::zero();
let projects = WhiteListedProjectAccounts::<T>::get();
for project in projects {
if Votes::<T>::contains_key(&project, &voter) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this check is unnecessary

substrate/frame/distribution/src/functions.rs Outdated Show resolved Hide resolved
substrate/frame/distribution/src/functions.rs Outdated Show resolved Hide resolved
substrate/frame/distribution/src/functions.rs Outdated Show resolved Hide resolved
substrate/frame/distribution/src/functions.rs Outdated Show resolved Hide resolved
substrate/frame/distribution/src/functions.rs Outdated Show resolved Hide resolved
&pot,
project.amount,
)
.expect("Funds Reserve Failed");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is invalid usage of expect. it should be a proof stating why it can never fail. in this case, it could be "pot_check already ensured enough funds; qed". However, it is still a bad code as people have to read the inner code of pod_check to check if this is true or not.

.expect("Funds Reserve Failed");

// Remove project from project_list
projects.retain(|value| *value != project);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is inefficient. you should just rewrite the loop to a filter

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T2-pallets This PR/Issue is related to a particular pallet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants