Add erc1155 #290
Add erc1155 #290
Conversation
|
this should probably be updated to include the namespace changes. here's a good reference pr for the 721 namespace changes #296 |
There was a problem hiding this comment.
Hey, @dewi-tim! I know you've been working on this for a while, so it's great to see this here! It looks really good :) I left some comments, questions, and discussions if you don't mind taking a look. Thank you for doing this, sir 🙏
| def uint_array(arr): | ||
| return list(map(uint, arr)) | ||
|
|
||
|
|
||
| def uarr2cd(arr): | ||
| acc = [len(arr)] | ||
| for lo, hi in arr: | ||
| acc.append(lo) | ||
| acc.append(hi) | ||
| return acc |
| # | ||
| # Unsafe recipients | ||
| # |
There was a problem hiding this comment.
A couple of these tests (correctly) are for safe recipients. Maybe we should change this to Non-account recipients or something like that?
Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>
|
Thanks for going over all this @andrew-fleming ! I'll get to fixing these |
Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>
Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>
|
Hey @dewi-tim! Just checking in. I know I provided a ton of suggestions and questions (sorry! 😅), but no pressure! If it's too much and/or you're too busy, just let me know :) |
|
Hey @andrew-fleming, things were busy, but I can resume work on this again soon, and the mountain of suggestions is nonetheless well appreciated! |
Co-authored-by: Martín Triay <martriay@gmail.com>
| pedersen_ptr: HashBuiltin*, | ||
| range_check_ptr | ||
| }(account: felt, id: Uint256) -> (balance: Uint256): | ||
| return ERC1155.balance_of(account, id) |
| @pytest.mark.asyncio | ||
| async def test_mint_batch_to_receiver(erc1155_factory): | ||
| erc1155, owner, _, receiver = erc1155_factory | ||
|
|
||
| recipient = receiver.contract_address | ||
|
|
||
| await signer.send_transaction( | ||
| owner, erc1155.contract_address, 'mintBatch', | ||
| [ | ||
| recipient, *uarr2cd(TOKEN_IDS), | ||
| *uarr2cd(MINT_AMOUNTS), DATA | ||
| ]) | ||
|
|
||
| execution_info = await erc1155.balanceOfBatch( | ||
| [recipient]*3, TOKEN_IDS).invoke() | ||
| assert execution_info.result.balances == MINT_AMOUNTS |
There was a problem hiding this comment.
again, i don't see the difference with test_mint_batch 🤔 . I understand the spirit is to test the acceptance of the token but I consider it implied in the above one, no?
There was a problem hiding this comment.
I wouldn't have said it is implied. In the first instance it is accepted by virtue of supporting the account interface, whereas in this case it is explicitly accepted by an IERC1155Receiver implementer. I could have in principle messed up either case and testing for just one wouldn't uncover that.
|
Thanks for your very thorough review! Sorry for the delay, I have some other commitments at the moment but I'll make these fixes surely by the end of next week, plausibly before Wednesday. |
|
Hey there! Considering how many and how breaking changes are in the upcoming bump to Cairo 0.10, I suggest you finish this PR with the old syntax and only then we migrate to the new one, so let's wait to agree on the PR being ready before doing so! |
Release v0.3.2
Co-authored-by: Martín Triay <martriay@gmail.com>
|
hey there @dewi-tim! what's the status on this? is this ready for review? |
|
Hi, sorry to reply so late. I think it was close to being ready for review, I created a new branch in the repo here that doesn't use safemath and consequently does fewer range checks while having more informative error messages and better deduplication of code. I'll give them both the once over today to check I haven't forgotten some whitespace/comment related things |
|
hey @dewi-tim! thank you very much for all the effort invested here. we decided to pick this up ourselves for the sake of speeding things up to release it during the current milestone. don't worry, you will be properly attributed in a new PR as well as in the changelog once we release. thanks again :) also, i wonder why you decided to drop safemath in the alternative PR |
Fixes #273.
This PR implements the ERC1155 library contract and an
ERC1155_Minable_Burnableimplementation for testing purposes (to expose the internal_mintand_burnmethods). It features a suite of tests which leverage the utils for caching and can be run in parallel. This work began at nethermind in Dec, was continued at Circularise and has roughly tracked the main repo since then. Sorry it took so long, and many thanks for your useful discussions @andrew-fleming.Please let me know of any changes needed and I'll try to get them done much sooner