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

feat: add router for contribution fees #282

Merged
merged 14 commits into from
Sep 22, 2023
Merged

feat: add router for contribution fees #282

merged 14 commits into from
Sep 22, 2023

Conversation

0xble
Copy link
Collaborator

@0xble 0xble commented Aug 29, 2023

Implements a crowdfund router that allows contributions to include a flat fee before being passed on to the crowdfund

@0xble 0xble added feature Add functionality crowdfund Related to crowdfund labels Aug 29, 2023
@height
Copy link

height bot commented Aug 29, 2023

This pull request has been linked to 2 tasks:

💡Tip: Add "Close T-3245" to the pull request title or description, to a commit message, or in a comment to mark this task as "Done" when the pull request is merged.

@codecov-commenter
Copy link

codecov-commenter commented Aug 29, 2023

Codecov Report

Patch coverage: 79.06% and project coverage change: +0.35% 🎉

Comparison is base (0a9fc2f) 60.61% compared to head (bcfd966) 60.96%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #282      +/-   ##
==========================================
+ Coverage   60.61%   60.96%   +0.35%     
==========================================
  Files          62       63       +1     
  Lines        2458     2475      +17     
  Branches      573      573              
==========================================
+ Hits         1490     1509      +19     
- Misses        772      775       +3     
+ Partials      196      191       -5     
Files Changed Coverage Δ
contracts/crowdfund/Crowdfund.sol 52.57% <0.00%> (-0.92%) ⬇️
contracts/crowdfund/ETHCrowdfundBase.sol 80.37% <ø> (ø)
contracts/gatekeepers/AllowListGateKeeper.sol 77.77% <0.00%> (-22.23%) ⬇️
contracts/gatekeepers/TokenGateKeeper.sol 88.88% <50.00%> (-11.12%) ⬇️
contracts/crowdfund/ContributionRouter.sol 100.00% <100.00%> (ø)
contracts/crowdfund/InitialETHCrowdfund.sol 86.56% <100.00%> (+4.87%) ⬆️
contracts/crowdfund/ReraiseETHCrowdfund.sol 78.63% <100.00%> (+2.59%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@arr00
Copy link
Contributor

arr00 commented Aug 29, 2023

Link T-3246

@arr00
Copy link
Contributor

arr00 commented Aug 29, 2023

We shouldn't use globals here for who can call the claim fees fn. Let's pass an owner on construction and have it be more generic wording.

@arr00
Copy link
Contributor

arr00 commented Aug 29, 2023

For simplicity, we don't actually need to know which type of crowdfund the user is contributing in this contract. It can just be a low level eth call with the given calldata to the given crowdfund. That may be a better approach.

@0xble
Copy link
Collaborator Author

0xble commented Aug 29, 2023

For simplicity, we don't actually need to know which type of crowdfund the user is contributing in this contract. It can just be a low level eth call with the given calldata to the given crowdfund. That may be a better approach.

It's more flexible. Would save us from having to redeploy if we were to add new crowdfund types. And could be adapted to add an optional to any function call, really. I say we go for it.

@0xble 0xble changed the title WIP: add router for contribution fees feat: add router for contribution fees Sep 5, 2023
@0xble 0xble marked this pull request as ready for review September 5, 2023 18:15
@arr00
Copy link
Contributor

arr00 commented Sep 8, 2023

Do we care to be able to switch the owner of the wrapper? I assume its fine like this.

@0xble
Copy link
Collaborator Author

0xble commented Sep 9, 2023

Do we care to be able to switch the owner of the wrapper? I assume its fine like this.

Since owner is going to be the DAO's multisig, it's unlikely to change. Worse case and it does change, can always deploy a new one with an updated owner and switch over to that.

@arr00 arr00 changed the base branch from dev to main September 11, 2023 19:56
arr00 and others added 2 commits September 18, 2023 17:37
* fix: mitigations contributor router

* gatekeepers mitigation

* initial work on fixing

* fix tests

* add invalid message value tests

* test gatekeeper

* fix gatekeep revert

* add docs

* Update comment contracts/crowdfund/ContributionRouter.sol

Co-authored-by: Brian Le <[email protected]>

* add delegate note

---------

Co-authored-by: Brian Le <[email protected]>
Copy link
Contributor

@arr00 arr00 left a comment

Choose a reason for hiding this comment

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

Let's add the audit then LGTM!

@0xble 0xble merged commit 9f0fd37 into main Sep 22, 2023
3 checks passed
@arr00 arr00 deleted the feat/contribution-fee branch September 22, 2023 16:22
@0xble 0xble restored the feat/contribution-fee branch September 27, 2023 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crowdfund Related to crowdfund feature Add functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants