WIP Royalty #72
WIP Royalty #72
Conversation
| @@ -0,0 +1,70 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| pragma solidity ^0.8.23; | |||
There was a problem hiding this comment.
Would suggest not using "^" in pragma for security reasons. This applies to all contracts in SPG repo.
You may also bump the version to the same version as protocol core
There was a problem hiding this comment.
Will create a ticket for updating all contracts to use 0.8.26 :)
| /// @param nftContractBeacon The address of the NFT contract beacon. | ||
| /// @custom:storage-location erc7201:story-protocol-periphery.RoyaltyWorkflows | ||
| struct RoyaltyWorkflowsStorage { | ||
| address nftContractBeacon; |
There was a problem hiding this comment.
what is the nftContractBeacon variable used for?
There was a problem hiding this comment.
good catch! We don’t actually need it in this contract. Will remove it :)
| ) external returns (uint256 snapshotId, uint256[] memory amountsClaimed) { | ||
| // Claims revenue for each specified currency token from the latest snapshot | ||
| IIpRoyaltyVault ancestorIpRoyaltyVault; | ||
| (snapshotId, amountsClaimed, ancestorIpRoyaltyVault) = _transferToVaultAndSnapshotAndClaimByTokenBatch( |
There was a problem hiding this comment.
_transferToVaultAndSnapshotAndClaimByTokenBatch - is this function claiming by token batch inside an larger function which is by snapshot batch?
Perhaps for simplicity - given these internal functions are a bit too big - can we make the sequence of elementary functions transferToVault, snapshot, claimByTokenBatch / claimBySnapshotBatch without any internal functions first? Just to make sure we don't miss anything and it's easier to follow.
Then at the end we can think of making internal functions to reduce code duplication.
There was a problem hiding this comment.
For example the function transferToVaultAndSnapshotAndClaimBySnapshotBatch could be:
transferToVault()
snapshot()
claimBySnapshotBatch()
using more elementary building blocks sequence. then at the end we think about internal functions.
Same thinking applies for the remaining functions.
There was a problem hiding this comment.
_transferToVaultAndSnapshotAndClaimByTokenBatch - is this function claiming by token batch inside an larger function which is by snapshot batch?
Good question! All the SPG functions let you claim multiple tokens at the same time. For the ones using claimBySnapshotBatch, we first take the latest snapshot and claim all tokens, then handle any additional unclaimed snapshots user provide. We set it up this way so that if user don't need to bundle the calls, they can use the core protocol to claim your snapshots directly.
One improvement we could add is making snapshotting optional in transferToVaultAndSnapshotAndClaimBySnapshotBatch and snapshotAndClaimBySnapshotBatch. That way, if taking the latest snapshot fails, users can still claim their unclaimed snapshots.
There was a problem hiding this comment.
Perhaps for simplicity - given these internal functions are a bit too big - can we make the sequence of elementary functions transferToVault, snapshot, claimByTokenBatch / claimBySnapshotBatch without any internal functions first? Just to make sure we don't miss anything and it's easier to follow.
Agreed, that would make the code a lot more readable
| amountsClaimed[i] += ancestorIpRoyaltyVault.claimRevenueOnBehalfBySnapshotBatch({ | ||
| snapshotIds: unclaimedSnapshotIds, | ||
| token: royaltyClaimDetails[i].currencyToken, | ||
| claimer: address(ancestorIpRoyaltyVault) |
There was a problem hiding this comment.
When doing the final call that will take the money from a vault to a royalty token holder the claimer can be any address in the world that holds royalty tokens. So claimer that we added in last protocol PR on the functions claimRevenueOnBehalfBySnapshotBatch and claimRevenueOnBehalfByTokenBatch are input from the app/outside and should be passed in to the SPG functions as the SPG has no way to know what this address is
Applies whenever these two functions above are called throughout SPG the RoyaltyWorkflows.sol
There was a problem hiding this comment.
I see, will add an extra claimer param to these functions 🫡
Description
Example:
This pr adds user login function, includes：
Test Plan
Example:
Related Issue
Example: Issue #123
Notes