Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

pallet-bounties | Sub-bounties Extn #7965

Closed
wants to merge 137 commits into from

Conversation

shamb0
Copy link
Contributor

@shamb0 shamb0 commented Jan 24, 2021

Sub-bounties Extn, Quick proto implemented for design discussion.

#7959

polkadot companion: paritytech/polkadot#2419

frame/bounties/src/lib.rs Outdated Show resolved Hide resolved
frame/bounties/src/lib.rs Outdated Show resolved Hide resolved
frame/bounties/src/lib.rs Outdated Show resolved Hide resolved
frame/bounties/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@xlc xlc left a comment

Choose a reason for hiding this comment

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

why are you modify the logic for master bounty other than check if it has active sub bounty?

frame/bounties/src/lib.rs Outdated Show resolved Hide resolved
frame/bounties/src/lib.rs Outdated Show resolved Hide resolved
frame/bounties/src/lib.rs Outdated Show resolved Hide resolved
frame/bounties/src/lib.rs Outdated Show resolved Hide resolved
frame/bounties/src/lib.rs Outdated Show resolved Hide resolved
frame/bounties/src/lib.rs Outdated Show resolved Hide resolved
frame/bounties/src/lib.rs Outdated Show resolved Hide resolved
frame/bounties/src/lib.rs Outdated Show resolved Hide resolved
frame/bounties/src/lib.rs Outdated Show resolved Hide resolved
frame/bounties/src/lib.rs Outdated Show resolved Hide resolved
frame/bounties/src/lib.rs Outdated Show resolved Hide resolved
frame/bounties/src/lib.rs Outdated Show resolved Hide resolved
frame/bounties/src/lib.rs Outdated Show resolved Hide resolved
@shawntabrizi
Copy link
Member

@shamb0

Thanks for the PR and the work here!

Unfortunately we would like to put this PR on hold until after the parachains release due to resource constraints reviewing this PR in depth, and the number of critical upcoming changes that will be pushed to Polkadot and Kusama in the next couple of weeks.

The main issue here is that this update affects an existing pallet which is used in Polkadot, Kusama, and other parachain runtimes.

As such, we really need to make sure that all of this is right, from the benchmarking, to the state transitions, and even the game theory.

In general, it would be good that external contributions as large as this be isolated into a separate pallet so they can be more easily merged without us feeling that there will be such an impact to our networks.

Maybe we can still merge this by simply putting the code into a new crate, but as is, we will have to wait.

Thanks!

@shamb0
Copy link
Contributor Author

shamb0 commented May 8, 2021

Thanks @shawntabrizi, Able to understand the integration complexity. Like the suggested solution of managing as separate crate. Will work on it and get back ASAP.

@paritytech paritytech deleted a comment from cla-bot-2021 bot Jun 3, 2021
@stale
Copy link

stale bot commented Jul 7, 2021

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Jul 7, 2021
@rrtti
Copy link

rrtti commented Jul 7, 2021

Awaiting further actions from @shawntabrizi's team for guidance on this - possibly to deploy only on Kusama for now, and what would be the best way to do so.

@stale stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Jul 7, 2021
@cla-bot-2021
Copy link

cla-bot-2021 bot commented Jul 8, 2021

User @rajesh-nodle, please sign the CLA here.

@stale
Copy link

stale bot commented Aug 7, 2021

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Aug 7, 2021
@rrtti
Copy link

rrtti commented Aug 8, 2021

Waiting for final review.

@stale stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Aug 8, 2021
@shamb0
Copy link
Contributor Author

shamb0 commented Aug 8, 2021

Thanks for interest on the PR :-)) , will update to upstream master and get back ASAP 👍

Signed-off-by: rajesh-nodle <rajesh@nodle.co>
Signed-off-by: rajesh-nodle <rajesh@nodle.co>
Signed-off-by: rajesh-nodle <rajesh@nodle.co>
@hanwencheng
Copy link

Any update on the PR? It seems only the CI need to be passed. We are waiting for this feature to be merged for Kusama Bounty #3

@stale
Copy link

stale bot commented Oct 9, 2021

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Oct 9, 2021
@rrtti
Copy link

rrtti commented Oct 10, 2021

waiting for new changes.

@stale stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Oct 10, 2021
@shamb0
Copy link
Contributor Author

shamb0 commented Oct 11, 2021

Hello Raul,

"New changes", Do you mean sync with upstream master ?

@gautamdhameja
Copy link
Contributor

To address some of the feedback and make the sub-bounties logic loosely coupled with the bounties pallet, here is a proposed high-level redesign:

  • Extract the sub-bounties logic in a separate pallet. Call it child-bounties pallet to avoid confusion (sub prefix is being used for substrate in some other places).
  • In the bounties pallet, add a trait and config type for child-bounty manager. This trait will have the linking logic for checking child-bounties before closing a bounty, etc.
  • In the runtime, we can assign the child-bounty pallet as the ChildBountyManager for Bounties pallet, or just () when not using child bounties.
  • This is similar to how session uses SessionManager.

Screenshot 2021-11-09 at 13 18 11

Thoughts?

cc/ @shawntabrizi

@shamb0
Copy link
Contributor Author

shamb0 commented Nov 11, 2021

Proposal sounds good for me 👍

@rrtti
Copy link

rrtti commented Nov 15, 2021

The Parity team will take over the redesign of the sub-bounties extension as a new pallet, based on @gautamdhameja's proposal above. They will extract the sub-bounties logic that is here in a separate pallet, add tests and becnhmarks and get it ready for review, deployment on Kusama and audit. I will proceed now to close this PR to continue the work somewhere else.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited
Projects
None yet
Development

Successfully merging this pull request may close these issues.