-
Notifications
You must be signed in to change notification settings - Fork 3
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
base: dev
Are you sure you want to change the base?
Conversation
constructor(address _allo, string memory _strategyName, bool _reviewEachStatus) | ||
BaseStrategy(_allo, _strategyName) | ||
{ | ||
REVIEW_EACH_STATUS = _reviewEachStatus; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Would it make sense to remove REVIEW_EACH_STATUS and always _processStatusRow ?
- Do the extensions need to have a constructor ?
There was a problem hiding this comment.
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
There was a problem hiding this 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? |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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
https://hackmd.io/@98cAvzvOSGOFP9kPfbp1YQ/rJk5NSrxyx