Add failing test for owner exclusion #4
Conversation
src/OperatorFilterer1155.sol
Outdated
| // Note that this still allows listings and marketplaces with escrow to transfer tokens if transferred | ||
| // from an EOA. | ||
| if (from == msg.sender) { | ||
| if (balanceOf(msg.sender, id) > 0) { |
There was a problem hiding this comment.
Curious, won't standard methods perform this check?
There was a problem hiding this comment.
Oh I've just seen that you left a similar comment. Wondering about the same thing.
| if (balanceOf(msg.sender) > 0) { | ||
| _; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Why is a non-zero token balance relevant? Shouldn't this be checked by the modifier receiving function?
There was a problem hiding this comment.
This is to skip the external call overhead to the registry and associated SLOADs there; overhead should be minimal in normal cases as the storage slot will then become warm for the balance check in the transfer method. Can add a comment about that or remove if you think the approach is problematic.
Edit: Actually, since the tx will fail anyway, this isn't necessary. Thanks!
There was a problem hiding this comment.
to skip the external call
Are you referring to the following?
There was a problem hiding this comment.
No just trying to understand what this is for.
If the modifier was to be applied to a transferFrom, it would revert anyways if the caller is not the owner or approved, right?
This is to skip the external call overhead to the registry and associated SLOADs there
Why would this need to call the registry?
There was a problem hiding this comment.
Exactly. My question was for @operatorfilterer and if the answer would have been positive I would have followed up with: #4 (review)
| @@ -26,13 +26,29 @@ contract OperatorFilterer { | |||
| } | |||
| } | |||
|
|
|||
| modifier onlyAllowedOperator() virtual { | |||
| modifier onlyAllowedOperator(address from) virtual { | |||
| // Check registry code length to facilitate testing in environments without a deployed registry. | |||
| if (address(operatorFilterRegistry).code.length > 0) { | |||
There was a problem hiding this comment.
this could be
if (from != msg.sender && address(operatorFilterRegistry).code.length > 0) {
OpenZeppelin M-01: Incomplete test coverage
This adds a failing test to demonstrate that token owners can be blocked by the proposed operator filter implementation.