-
Notifications
You must be signed in to change notification settings - Fork 83
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: creator reward recipient per token #278
Conversation
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
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.
I see 2 new tests – does the 3rd test for contract balance already exist?
Approve, but would be nice to update a few small syntax things.
/// @dev The creator is not enforced to set a funds recipient address for a specific token or contract-wide, | ||
/// so in the case of both the reward recipient is set to the creator's contract, | ||
/// which can be withdrawn by the creator via the `withdrawRewards` function. | ||
function getCreatorRewardRecipient(uint256 tokenId) public view returns (address payable) { |
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.
@kulkarohan can we check the front-end and subgraph to make sure there are no calls to this function?
Would it make sense to make this internal?
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.
confirmed - no frontend and subgraph usage of this function (yet)
i believe however frontend is planning to pin the creator reward recipient address on each token page, so i think having it public to fetch would make their lives easier, at little cost to us since we need it anyways.
but can confirm w them and if they're not can just make this internal
removed payable cast and updated tests |
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.
minor suggested improvements
it's a bit confusing here that we are pulling the funds recipient from the royalty recipient; why not create a specific funds recipient per token, and just have royalty recipient read from that to determine royalty payout address? |
c987b66
to
3ade5a5
Compare
closing & reopening as prerelease branch |
jk |
will deploy and add changeset once we're good on reviews
will deploy and add changeset once we're good on reviews
* feat: erc20 minter * feat: Premint with ERC-20 * refactor: revert with custom error * chore: run lint * refactor: revert invalid premint version * refactor: emit firstMinter in PremintedV2 event * refactor: remove previous local variables * chore: add referral fetch tests * chore: update header url * chore: remove .vscode * refactor: update erc20 slippage error name * docs: update requestMint natspec * refactor: rename address minter in EncodedPremintConfig * refactor: update premint with erc20 slippage error * chore: run lint * feat: modifiable reward recipient address & support for deterministic deploy * refactor: group constants * docs: update storage vars natspec * fix: update IERC20Minter natspec for events and errors * fix: change erc-20 to erc20 * feat: rename zoraRewardAddress to zoraRewardRecipientAddress * docs: update natspec * chore: add ERC20 minter * chore: add ERC20 minter deterministic config * Update Subgraph With Deployed ERC20 Minter Addresses (#278) * feat: add erc20Minter deployed addresses to configs * fix: remove abi changes * feat: add changeset --------- Co-authored-by: Rohan Kulkarni <kulkarohan@gmail.com>
will deploy and add changeset once we're good on reviews