I created the PR already, will try to get it merged soon but in the meantime you can try with the fix from the PR here
I included your failing test and with this fix it passes. I’m glad you found this problem, it really shows you’re doing a detailed review, thanks @WaiChung !
Hey @stoicdev0 I’ve a couple of questions regarding implementation.
I cannot find any documentation around the removeItemByIndex method used in the acceptAsset and rejectAsset functions. I assume it’s of O(n) complexity. Do you think it makes sense to do it in O(1) time? This way perhaps we can remove the 128 upper bound on the _pendingAssets list and make it consistent with how _addAssetsToToken is designed?
Do you think it makes sense for us to add a unit test to handle the race condition below?
We addressed the race condition where unintended resource managed by index could be accepted/rejected by adding ID of the resource expected to be located at the given index to the calls, this way a check can be added to ensure only the intended resource is accepted/rejected.
The implementation of removeItemByIndex is here it’s O(1) because we know the index (and it’s actually the reason we require indexes, so we don’t have to loop to search). The 128 limitation is just for practical reasons, we consider it would be unmanageable for UIs to go beyond that.
We have tests for this on our original repo but I just see they are not updated in the EIP repo. Let me add them and ping you. We have 3 tests for accept, reject and reject all.
The implementation method would need to send 10k AssetAddedToToken Events, right? This could cost 15 million gas / around 0.3eth at current gas price. In many cases it will also also not be possible to implement this in a single transaction because of the block gas limit. Considering this will likely be the most common use case, a O(1) instead of O(N) event seems necessary to me and would allow adding assets to all tokens pretty much for free. Not including it from the start may result in a similar messy situation like with ERC-2309, where some indexers support it and some don’t. Adding a second interface to this EIP or using a less opinionated event like AssetAddedToAllTokens are also options.
It’s a great point. What about replacing AssetAddedToToken with AssetAddedToTokens? The second one receiving the range as you recommend.
I fear if we have 2 events, not everyone will track them both. The ranged version still does the job for a single token.
Yes, I think that will work. With the range it will also be good to address wether tokenIds need to exist or not. If they don’t have to exist, are they ignored or is the asset still added to the token after the mint happened (probably not good for indexers).
Another question I’ve had is wether you always have to emit both an AssetAddedToToken and AssetAccepted Event in order to add an asset to the active list. It could make sense to only require AssetsAccepted and allow skipping AssetAddedToToken especially during the mint process to add the initial asset. It would also be possible to allow assets being added to the active list without emitting any events during the mint, but this might be bad for indexers.
We feel is more flexible than a range, and you can still emit it with a few thousands before hitting gas limits.
Regarding the 2 events, you said it your self: it would be bad for indexers. We wouldn’t be able to distinguish pending assets from active assets. And only pending can be rejected, so it’s an important distinction.