cairo-contracts icon indicating copy to clipboard operation
cairo-contracts copied to clipboard

Published 20 hours ago • verified publisher icon OpenZeppelin

Add erc1155

Open dewi-tim opened this issue 2 years ago • 7 comments

Fixes #273.

This PR implements the ERC1155 library contract and an ERC1155_Minable_Burnable implementation for testing purposes (to expose the internal _mint and _burn methods). 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

dewi-tim avatar Apr 28 '22 14:04 dewi-tim

this should probably be updated to include the namespace changes. here's a good reference pr for the 721 namespace changes https://github.com/OpenZeppelin/cairo-contracts/pull/296

koloz193 avatar May 06 '22 15:05 koloz193

Thanks for going over all this @andrew-fleming ! I'll get to fixing these

dewi-tim avatar May 12 '22 09:05 dewi-tim

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 :)

andrew-fleming avatar Jun 03 '22 02:06 andrew-fleming

Hey @andrew-fleming, things were busy, but I can resume work on this again soon, and the mountain of suggestions is nonetheless well appreciated!

dewi-tim avatar Jun 07 '22 08:06 dewi-tim

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.

dewi-tim avatar Sep 01 '22 08:09 dewi-tim

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!

martriay avatar Sep 07 '22 01:09 martriay

hey there @dewi-tim! what's the status on this? is this ready for review?

martriay avatar Oct 06 '22 18:10 martriay

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

dewi-tim avatar Oct 17 '22 07:10 dewi-tim

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

martriay avatar Nov 24 '22 20:11 martriay