ERC1155 and ERC721 Floor Wrappers #79
Conversation
| } | ||
|
|
||
| function initialize(address tokenAddr) external { | ||
| if (initialized || msg.sender != factory) { |
There was a problem hiding this comment.
I know this is for being on the "safe side" but I don't think initialized is needed here. With msg.sender + token != address(0) should be enough.
There was a problem hiding this comment.
How about going further and only relying on msg.sender != factory? Will be safe since the factory will only ever call this function once immediately after creation.
There was a problem hiding this comment.
token != address(0) has practically no additional cost (since the store has to be written too anyway), you are right that msg.sender != factory should be enough, but having an extra "correctness" check doesn't hurt in this case.
| * @param data Data to pass to ERC-1155 receiver. | ||
| * @notice Users must first approve this contract address on the ERC-1155 contract. | ||
| */ | ||
| function deposit(uint256[] memory tokenIds, uint256[] memory tokenAmounts, address recipient, bytes calldata data) |
There was a problem hiding this comment.
Why not just use the ERC1155 callback for this? With this approach you always need to do an approve, and by using a callback you can also support wrapping by just sending to this contract.
There was a problem hiding this comment.
This function signature is nice because it allowed deposits to be swapped immediately.
To enable this with the ERC1155 Received callback I'd need to do some manual parsing of the
data which feels messy.
There was a problem hiding this comment.
Keep in mind decoding data is a pattern that we already use heavily on Niftyswap, you can implement it in a "clean" way be using abi.decode and a struct definition.
There was a problem hiding this comment.
I was worried about tightly coupling Niftyswap structs to the wrapper implementation. Found a way around it so I prefer this approach now too.
ScreamingHawk
force-pushed
the
floor-wrappers
branch
2 times, most recently
from
19cba2b to
c9e2f37
Compare
ScreamingHawk
force-pushed
the
floor-wrappers
branch
from
80281fb to
1506c5c
Compare
ScreamingHawk
force-pushed
the
floor-wrappers
branch
from
acd4c4f to
c89bf42
Compare
ScreamingHawk
force-pushed
the
floor-wrappers
branch
from
0185fbe to
8316aab
Compare
| } | ||
|
|
||
| if (_ids.length != 1) { | ||
| revert InvalidERC1155Received(); |
There was a problem hiding this comment.
Is this code reachable? the wrapper implements a single token ID, so you shouldn't be able to test this.
If so then it should be better to use an assert(_ids.length == 1).
There was a problem hiding this comment.
It could be called with multiple token ids if the amounts are 0.
safeBatchTransferFrom(_from, _to, [1, 2, 3], [0, 0, 0])
We could override the transfer functions and check for id length there instead. That would be more gas efficient too
There was a problem hiding this comment.
^ In progress. Blocked by 0xsequence/erc-1155#101
| _deposit(_amounts, _data); | ||
| } else if (msg.sender == address(this)) { | ||
| // Withdraw | ||
| if (_ids.length != 1) { |
There was a problem hiding this comment.
Same here, this should be an assert
|
This PR is becoming stale. Shall we merge this @Agusx1211 @acrylix @PhABC ? |
Wrapper contracts to enable collection wide "floor" trading for ERC1155 and ERC721 tokens via Niftyswap.