onchain metadata support for multi delegate #25
Conversation
contracts/ERC20MultiDelegate.sol
Outdated
| require((sources[transferIndex] >> 160) == 0, "Upper 96 bits of source uint256 must be zero"); | ||
| require( | ||
| (sources[transferIndex] >> 160) == 0, | ||
| "Upper 96 bits of source uint256 must be zero" |
There was a problem hiding this comment.
These should be custom errors.
contracts/ERC20MultiDelegate.sol
Outdated
| @@ -108,15 +117,22 @@ contract ERC20MultiDelegate is ERC1155, Ownable { | |||
| for ( | |||
| uint transferIndex = 0; | |||
| transferIndex < Math.max(sourcesLength, targetsLength); | |||
There was a problem hiding this comment.
IIRC one of the gas optimisation suggestions was to calculate the max outside the loop.
contracts/ERC20MultiDelegate.sol
Outdated
| function setUri(string memory uri) external onlyOwner { | ||
| _setURI(uri); | ||
| emit MetadataURIUpdated(uri); | ||
| function setUri(string memory /*uri*/) external view onlyOwner { |
There was a problem hiding this comment.
Can we just remove this instead - and the ownership functionality with it?
contracts/ERC20MultiDelegate.sol
Outdated
| ) { | ||
| resolvedName = _resolvedName; | ||
| } catch { | ||
| return ""; |
There was a problem hiding this comment.
A failure to get the primary name shouldn't result in all the metadata being empty. Perhaps just return the hex-encoded address instead?
contracts/ERC20MultiDelegate.sol
Outdated
| "text(bytes32,string)", | ||
| [namehash, "avatar"] | ||
| ); | ||
| (bytes memory result, ) = metadataResolver.resolve(encodedName, data); |
contracts/ERC20MultiDelegate.sol
Outdated
| return string.concat("data:application/json;base64,", json); | ||
| } | ||
|
|
||
| function trim( |
There was a problem hiding this comment.
Copying the address-to-hex implementation into /utils/HexUtils.sol, and removing the 0x prefix, would remove the need for this function.
There was a problem hiding this comment.
added a new HexUtils method, which converts address to hex string without hex prefix. ptal if that's okay.
contracts/ERC20MultiDelegate.sol
Outdated
| address source = address(0); | ||
| address target = address(0); | ||
| if (transferIndex < sourcesLength) { | ||
| require((sources[transferIndex] >> 160) == 0, "Upper 96 bits of source uint256 must be zero"); | ||
| if ((sources[transferIndex] >> 160) != 0) |
There was a problem hiding this comment.
Please run lint, and always use braces on if/else blocks.
contracts/ERC20MultiDelegate.sol
Outdated
| address source = address(0); | ||
| address target = address(0); | ||
| if (transferIndex < sourcesLength) { | ||
| require((sources[transferIndex] >> 160) == 0, "Upper 96 bits of source uint256 must be zero"); | ||
| if ((sources[transferIndex] >> 160) != 0) | ||
| revert Upper96BitsNotZero(); |
There was a problem hiding this comment.
This should probably be less implementation-specific, like InvalidDelegateAddress.
contracts/ERC20MultiDelegate.sol
Outdated
| bytes( | ||
| string.concat( | ||
| '{"name": "', | ||
| resolvedName, |
There was a problem hiding this comment.
This and imageUri need to be escaped.
There was a problem hiding this comment.
Do you think double quotes and backslash escaping will be enough?
There was a problem hiding this comment.
Looks like it: https://www.json.org/json-en.html
No description provided.