-
Notifications
You must be signed in to change notification settings - Fork 0
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
3078 - Series indexer contract #44
base: main
Are you sure you want to change the base?
Conversation
Implement version 1 Better naming f
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.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
solhint
GC: Use Custom Errors instead of require statements
require(proposedArtistAddr != address(0), "Invalid address"); |
GC: Use Custom Errors instead of require statements
require(!seriesPendingCoArtist[seriesID][proposedArtistID], "Already proposed"); |
GC: Use Custom Errors instead of require statements
require(seriesPendingCoArtist[seriesID][proposedArtistID], "No proposal exists"); |
GC: Use Custom Errors instead of require statements
require(artistID != 0, "Not an artist"); |
GC: Use Custom Errors instead of require statements
require(seriesPendingCoArtist[seriesID][artistID], "No pending proposal"); |
GC: Use Custom Errors instead of require statements
require(artistID != 0, "Not an artist"); |
GC: Use Custom Errors instead of require statements
require(!ownerRightsRevokedForArtistID[artistID], "Already revoked"); |
GC: Use Custom Errors instead of require statements
require(artistID != 0, "Not an artist"); |
GC: Use Custom Errors instead of require statements
require(ownerRightsRevokedForArtistID[artistID], "Not revoked"); |
GC: Use Custom Errors instead of require statements
require(newAddress != address(0), "Invalid new address"); |
Error message for require is too long: 37 counted / 32 allowed
require(addressToArtistID[newAddress] == 0, "Address already assigned to an artist"); |
GC: String exceeds 32 bytes
require(addressToArtistID[newAddress] == 0, "Address already assigned to an artist"); |
GC: Use Custom Errors instead of require statements
require(addressToArtistID[newAddress] == 0, "Address already assigned to an artist"); |
GC: Use Custom Errors instead of require statements
require(oldAddress != address(0), "Invalid artistID"); |
GC: Use Custom Errors instead of require statements
require(isCallerArtist || isOwnerWithRights, "Not authorized to update address"); |
Error message for revert is too long: 39 counted / 32 allowed
revert("One of the artists revoked owner rights"); |
GC: String exceeds 32 bytes
revert("One of the artists revoked owner rights"); |
GC: Use Custom Errors instead of revert statements
revert("One of the artists revoked owner rights"); |
a8851ad
to
abf3af4
Compare
contracts/SeriesIndexer.sol
Outdated
|
||
contract SeriesIndexer is Ownable.Ownable { | ||
// Counter | ||
uint256 private nextSeriesID = 1; |
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.
Can you use the length
of existing series map so we don't need to maintain this counter?
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.
this's not possible since the mapping in solidity is a simple hash table without key management.
contracts/SeriesIndexer.sol
Outdated
contract SeriesIndexer is Ownable.Ownable { | ||
// Counter | ||
uint256 private nextSeriesID = 1; | ||
uint256 private nextArtistID = 1; |
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.
Same has above comment but for the artist map.
contracts/SeriesIndexer.sol
Outdated
uint256 private nextArtistID = 1; | ||
|
||
struct Series { | ||
string metadata; |
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 suppose this field is for series metadata source, so it should store the uri so the name should be metadataURI
contracts/SeriesIndexer.sol
Outdated
|
||
struct Series { | ||
string metadata; | ||
string contractTokenData; |
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.
Same as above comment, it should be tokenDataURI
contracts/SeriesIndexer.sol
Outdated
// Artist Management | ||
mapping(uint256 => address) private artistIDToAddress; | ||
mapping(address => uint256) private addressToArtistID; | ||
mapping(uint256 => bool) private ownerRightsRevokedForArtistID; |
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 think this should be on series level instead of artist level. We could also provide a feature for revoking all series from an artist.
contracts/SeriesIndexer.sol
Outdated
// Internal Helper Functions | ||
// ------------------------ | ||
|
||
function _checkOwnerOrArtist(uint256 seriesID) internal view { |
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.
Should pass the address instead of assume the msg.sender
. The caller should determine which one should be passed, that will be more extensible and readable. If we just want to stick with the msg.sender
, we might need a better name like _checkMsgSenderIsOwnerOrArtist
contracts/SeriesIndexer.sol
Outdated
} | ||
} | ||
|
||
function _checkArtist(uint256 seriesID) internal view { |
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.
The same here.
contracts/SeriesIndexer.sol
Outdated
uint256 indexed seriesID, | ||
uint256[] artistIDs, | ||
string metadata, | ||
string seriesContractTokenCID |
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 would prefer using something like URI
instead of CID
that we need to stick to ipfs. Using URI
could be better for extensibility later on.
contracts/SeriesIndexer.sol
Outdated
require(length <= 50, "Batch size too large"); // Prevent DOS attacks | ||
|
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.
What does this mean for prevent DOS attacks?
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.
that mean if someone try to put really large number of list, the contract will deny to service. It's not quite of an attack I think.
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.
the words is removed anyway
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.
So that could be out of gas since it could reach the block gas limit so i don't consider it as "denial of service"
contracts/SeriesIndexer.sol
Outdated
seriesExists(seriesID) | ||
onlyOwnerOrArtist(seriesID) | ||
{ | ||
Series storage series = seriesDetails[seriesID]; |
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.
Should use memory
contracts/SeriesIndexer.sol
Outdated
Series storage series = seriesDetails[seriesID]; | ||
for (uint256 i = 0; i < series.artistIDs.length; i++) { | ||
uint256 artistID = series.artistIDs[i]; | ||
isArtistIDInSeries[seriesID][artistID] = false; |
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.
It could be redundant since we did it in the _removeSeriesFromArtist
contracts/SeriesIndexer.sol
Outdated
for (uint256 i = 0; i < series.artistIDs.length; i++) { | ||
uint256 artistID = series.artistIDs[i]; | ||
isArtistIDInSeries[seriesID][artistID] = false; | ||
_removeSeriesFromArtist(artistID, seriesID); |
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.
Why don't we call the function _removeArtistFromSeries
as well if we tend to remove the artist from series?
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.
nvm, you did it when deleting the storage below.
|
||
_ensureArtistHasID(proposedArtistAddr); | ||
uint256 proposedArtistID = addressToArtistID[proposedArtistAddr]; | ||
|
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.
It looks like you don't check the proposed address has been already artist for this series.
contracts/SeriesIndexer.sol
Outdated
} | ||
|
||
Series storage series = seriesDetails[seriesID]; | ||
series.artistIDs.push(artistID); |
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.
This logic could be presented already in _addArtistsToSeries
, could it be reused?
contracts/SeriesIndexer.sol
Outdated
if (artistID == 0) { | ||
revert NotAnArtistError(msg.sender); | ||
} | ||
if (!ownerRightsRevokedForArtistID[artistID]) { |
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.
Redundant parentheses
pragma solidity ^0.8.0; | ||
|
||
import "@openzeppelin/contracts/access/Ownable.sol" as Ownable; | ||
import "@openzeppelin/contracts/utils/structs/BitMaps.sol"; |
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.
global import of path @openzeppelin/contracts/utils/structs/BitMaps.sol is not allowed. Specify names to import individually or bind all exports of the module into a name (import "path" as Name)
contracts/SeriesIndexer.sol
Outdated
* @dev Creates a new series | ||
*/ | ||
function addSeries( | ||
address[] calldata artistAddrs, |
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.
Can you make clear variable name instead of abbreviation? This is the external function that would be seen publicly so I would suggest using clear name.
contracts/SeriesIndexer.sol
Outdated
* @dev Batch creation of series | ||
*/ | ||
function batchAddSeries( | ||
address[][] calldata artistsArray, |
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.
This name could be confusing because of the word array
, would just artists
or seriesArtists
be better?
contracts/SeriesIndexer.sol
Outdated
) external returns (uint256[] memory) { | ||
_validateBatchParameters(artistsArray.length, metadataURIs, tokenIDsMapURIs); | ||
|
||
uint256[] memory createdSeriesIDs = new uint256[](artistsArray.length); |
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.
Maybe just seriesIDs
is good enough.
contracts/SeriesIndexer.sol
Outdated
if (artistAddrs.length == 0) { | ||
revert NoArtistsForSeriesError(); | ||
} | ||
if (msg.sender == owner()) { |
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.
Could write a private function _isCallerOwner
for removing duplicated checks.
contracts/SeriesIndexer.sol
Outdated
/** | ||
* @dev Ensures all provided artist addresses have not revoked owner rights | ||
*/ | ||
function _validateArtistsNotRevoked(address[] memory artistAddrs) internal view { |
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.
Could be _ensureNoArtistsRevokedOwnerRights
, that will be more precise.
contracts/SeriesIndexer.sol
Outdated
/** | ||
* @dev Validates that artists can only add series that only have themselves as artist | ||
*/ | ||
function _validateOnlySelfAsArtist( |
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.
Maybe just _ensureCallerIsOnlyArtist
is good enough.
_validateOnlySelfAsArtist(artistAddrs); | ||
} | ||
uint256 seriesID = nextSeriesID++; | ||
_validateMetadataAndTokenURI(metadataURI, tokenIDsMapURI); |
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.
Should call this before the seriesID
increment.
contracts/SeriesIndexer.sol
Outdated
mapping(uint256 => BitMaps.BitMap) private artistOwnerRightsRevokedBitMap; | ||
|
||
// Series Management | ||
mapping(uint256 => Series) private seriesDetails; |
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'm wondering why isn't it just series
according to the artists
declaration above?
contracts/SeriesIndexer.sol
Outdated
|
||
// Artist Management | ||
mapping(uint256 => Artist) private artists; | ||
mapping(address => uint256) private addressToArtistID; |
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.
Could be artistAddressToID
uint256 seriesID, | ||
string memory metadataURI, | ||
string memory tokenIDsMapURI | ||
) internal seriesExists(seriesID) onlyOwnerOrArtist(seriesID) { |
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'd preferring using the modifiers in public/external function to leverage the visibility of them, we also could prevent potential gas waste when calling internal function multiple times.
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 put this into internal function since the check unit is per series so we can reuse the modifier. If we want the modifier only at external function level we'll need to create both single and plural unit modifiers. It's ok to me, just confirm that's what we want to do.
*/ | ||
function _deleteSeries(uint256 seriesID) | ||
internal | ||
seriesExists(seriesID) |
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.
Same here
contracts/SeriesIndexer.sol
Outdated
/** | ||
* @dev Validates batch parameters for series creation | ||
*/ | ||
function _validateBatchParameters( |
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.
This is quite ambiguous to me, maybe _validateSeriesWriteBatch
could be better. It describes this is for series write operation in batch.
contracts/SeriesIndexer.sol
Outdated
tokenIDsMapURIs.length | ||
); | ||
} | ||
if (seriesCount == 0) { |
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.
Should check these count first since it's cheaper.
And using in-memory variables to avoid access array length
multiple times, it could be better in readability and gas (not so sure)
contracts/SeriesIndexer.sol
Outdated
/** | ||
* @dev Allows an artist to remove themselves from a series | ||
*/ | ||
function removeSelfFromSeries(uint256 seriesID) external seriesExists(seriesID) onlyArtist(seriesID) { |
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.
ChatGPT suggests a name resignFromSeries
that's pretty good to me, It emphasizes the voluntary, you could consider.
contracts/SeriesIndexer.sol
Outdated
} | ||
|
||
// Remove series from artist's list | ||
_removeSeriesFromArtist(artistID, seriesID); |
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.
Why don't we just create a function like _removeArtistFromSeries
to grab above logic and make this function more structured?
BTW, this name _unlinkSeriesFromArtist
suggested by chatGPT is pretty good to emphasize the function purpose.
contracts/SeriesIndexer.sol
Outdated
*/ | ||
function ownerUpdateSeriesArtists( | ||
uint256 seriesID, | ||
address[] calldata artistAddrs |
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.
Should use clear name instead of abbreviation.
contracts/SeriesIndexer.sol
Outdated
// Add new artists | ||
_addArtistsToSeries(seriesID, artistAddrs); | ||
|
||
emit SeriesUpdated( |
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.
This event could be unclear. We just update the series artists so we don't need to fire the event with metadata URI or token data URI, I'd suggest to create new event if needed.
|
||
// Remove current artists | ||
Series storage series = seriesDetails[seriesID]; | ||
for (uint256 i = 0; i < series.artistIDs.length; i++) { |
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.
This block of code is for removing artists from series, I'd suggest write a private/internal function to do that. The ownerUpdateSeriesArtists
is basically call 2 internal functions to remove and add artists again.
contracts/SeriesIndexer.sol
Outdated
// Remove series from artist's list | ||
_removeSeriesFromArtist(artistID, seriesID); | ||
|
||
emit SeriesUpdated( |
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.
Same here. If you just want to fire a general event for series update, you might just want to pass the seriesID
and the client should use the seriesID to query the details from the contract. Adding more unrelated parameters in the event could cost more gas and also make it more confusing.
contracts/SeriesIndexer.sol
Outdated
*/ | ||
function proposeCoArtist( | ||
uint256 seriesID, | ||
address proposedArtistAddr |
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.
Shouldn't use the abbreviation.
contracts/SeriesIndexer.sol
Outdated
/** | ||
* @dev Adds a pending co-artist request | ||
*/ | ||
function _addPendingCoArtistRequest(uint256 artistID, uint256 seriesID) 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.
We could name it better like _addCoArtistProposal
contracts/SeriesIndexer.sol
Outdated
/** | ||
* @dev Removes a pending co-artist request | ||
*/ | ||
function _removePendingCoArtistRequest(uint256 artistID, uint256 seriesID) 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.
This should be named as _removeCoArtistProposal
contracts/SeriesIndexer.sol
Outdated
string metadataURI; // IPFS hash or similar identifier for series metadata | ||
string contractTokenDataURI; // Token-related data for the series | ||
uint256[] artistIDs; // List of artists associated with this series | ||
uint256[] pendingCoArtists; // List of pending co-artists |
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.
Maintaining a pending artist list within a series is not a good choice to me. The reason is the pending artist is just a temporary data acts as an intermediate stage for artist proposal. In other words, it doesn't really relate to the series. The series should have artist, metadata and token data, that's what we need for a series. When query the series from contract, we don't care about the co-artist since it's not official one.
Instead, I'd suggest maintain a pending co-artist as separate data, out of the series.
contracts/SeriesIndexer.sol
Outdated
struct Artist { | ||
address artistAddress; // Artist's wallet address | ||
uint256[] seriesIDs; // List of series associated with this artist | ||
uint256[] pendingCoArtistSeries; // List of pending co-artist series |
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.
Similar to this comment.
contracts/SeriesIndexer.sol
Outdated
/** | ||
* @dev Revokes owner rights for the calling artist | ||
*/ | ||
function revokeOwnerRightsForArtist() external { |
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'd suggest using the name revokeContractOwnerRights
contracts/SeriesIndexer.sol
Outdated
/** | ||
* @dev Approves owner rights for the calling artist | ||
*/ | ||
function approveOwnerRightsForArtist() external { |
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'd suggest using the name approveContractOwnerRights
contracts/SeriesIndexer.sol
Outdated
/** | ||
* @notice Checks if the given `artistAddr` has revoked owner rights | ||
*/ | ||
function ownerRightsRevoked(address artistAddr) external view returns (bool) { |
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.
Could be hasArtistRevokedOwnerRights
for more clear.
Also shouldn't use abbreviation for parameter.
/** | ||
* @notice Returns all artist IDs for a given series | ||
*/ | ||
function getSeriesArtistIDs(uint256 seriesID) external view returns (uint256[] memory) { |
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.
Can we consider to expose a function to getSeries
instead of expose multiple one that just easily achieved the same thing by just one 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.
I prefer returning the basic data type instead of struct so it could be used easier by client.
Description
Document of the flow design and/or figma design