carbonable-labs / cairo-erc-7498

NFT Redeemables
Apache License 2.0
6 stars 8 forks source link

Implement EIP7498 as a Cairo 2.6 component #4

Closed saimeunt closed 7 months ago

saimeunt commented 7 months ago

This is my WIP implementation of EIP 7498, I need some directions to finish this.

I tried to replicate the reference Solidity implementation 1:1 but this proved to be much harder than anticipated.

Here is some questions that remain to be clarified:

tekkac commented 7 months ago
  • I struggled with the correct way to store complex structs nested with arrays in Cairo, this is a no brainer in Solidity. I ended up implementing read/write using several LegacyMap but I'm not sure if this is the best way to proceed.

Indeed, don't use arrays but other structures, options are:

  • I've had hard times with Cairo move semantics and probably ended up relying on cloning structs much more than needed. In particular I was unable to access struct members of a snapshot (only .len() and .clone() worked somehow).

You should check in the cairo book about arrays and spans. For instance, a way to loop over a span is shown at the bottom of this page

  • The solidity implementation is receiving/sending native tokens, how to replicate this with Starknet? Should I use the fee token along with ERC20 logic?

Indeed, use ETH and/or STRK with erc20 logic. Mainnet addresses are found here

  • The Solidity implementation relies on inheritance/method overriding to handle the burn logic, I'm unsure how to implement this behavior in Cairo?

As inheritance is "handled" with components/impl: You need to create a custom impl, either in the component or preset, to handle the additional logic.

  • Some very basic tests are added to make sure my code at least compiled correctly.
  • A thorough review and help to head in the right direction would be highly appreciated πŸ‘

Thanks for the basic tests! I agree. Tests are not in the scope of this issue to ease the amount of work needed so it's considered "bonus" at this point.

saimeunt commented 7 months ago

@tekkac This PR is now ready for review, it still needs polishing on code quality and performance (having issues with move/ownership and complex nested structs/arrays) but overall the Solidity reference implementation behavior is replicated along with integration tests.

Concerning storage I went along with the array emulation technique with LegacyMaps, it's working as expected.

I think I can drop support for ItemType::NATIVE altogether because there's no such concept in Starknet, one should use ItemType::ERC20 with ETH/STRK tokens.

I removed the useInternalBurn / internalBurn logic to rely on the burn mechanism found in the latest implementation of ERC7498: https://github.com/ProjectOpenSea/redeemables/blob/main/src/lib/ERC7498NFTRedeemables.sol#L209

Fixes #1 Fixes #3

saimeunt commented 7 months ago

@tekkac Thank you for the review, I will polish the initial implementation with your comments in a separate PR πŸ‘Œ