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

bounty -> hackathon #46

Draft
wants to merge 5 commits into
base: dev
Choose a base branch
from
Draft

bounty -> hackathon #46

wants to merge 5 commits into from

Conversation

thelostone-mc
Copy link
Member

@thelostone-mc thelostone-mc commented Oct 23, 2024

constructor(address _allo, string memory _strategyName, bool _reviewEachStatus)
BaseStrategy(_allo, _strategyName)
{
REVIEW_EACH_STATUS = _reviewEachStatus;
Copy link
Member Author

Choose a reason for hiding this comment

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

^ @0xOneTony

  • Would it make sense to remove REVIEW_EACH_STATUS and always _processStatusRow ?
  • Do the extensions need to have a constructor ?

Choose a reason for hiding this comment

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

  • Hmmm i think that if someone doesnt want to do some extra processing for each individual then its much cheaper to have REVIEW_EACH_STATUS = false and fill in the statuses, it gives a bit of flexibility.
  • we used a constructor here because we have REVIEW_EACH_STATUS immutable, usually extensions shouldn't need a constructor, they could use init function

Copy link

@0xOneTony 0xOneTony left a comment

Choose a reason for hiding this comment

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

Great job! 🔥
The BountyExtension is a good idea, however if you include the allocation and distribution logic it makes it more of a complete strategy example rather an extension. I think it would be better to keep the BountyExtension without the allocate, distribute, and _processRecipient logic, so the extension will handle the definition of bounties and creating them.

Then you could have a BountyBaseStrategy which will use the RecipientExtension and the BountyExtension and it will have the basic allocate, distribute and _processRecipient logic.

From there you could have different flavors of BountyStrategies either by making use of the BountyExtension or by inheriting the BountyBaseStrategy.

/// ===============================

function __BountyExtension_init() internal {
// todo: no code?

Choose a reason for hiding this comment

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

yes! in this case if you dont have to initialize anything you can leave it

/// ===============================
/// ========= Initialize ==========
/// ===============================
function initialize(uint256 _poolId, bytes memory _data) external override {

Choose a reason for hiding this comment

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

dont forget to also initialize the RecipientExtension

/// ========= Initialize ==========
/// ===============================

function initialize(uint256 _poolId, bytes memory _data) external override {

Choose a reason for hiding this comment

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

dont forget to also initialize the RecipientExtension

@thelostone-mc thelostone-mc changed the title hackathon bounty -> hackathon Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants