search

public-awesome / cw-ics721

CosmWasm IBC NFT Transfers
MIT License
59 stars 31 forks source link

Ics721 package for better re-usability #67

Closed taitruong closed 1 year ago

taitruong commented 1 year ago

Main ICS721 logic has been moved to ics721 package. Main changes are by defining a struct and 2 traits:

Both traits provides default implementations. Especially this one is noteworthy - since it uses by default cw721-base: https://github.com/arkprotocol/ark-ics721/blob/ics721-package/packages/ics721/src/execute.rs#L287-L295

As a result the ics721-base contract itself is pretty simple, with lib.rs using default trait implementations: https://github.com/arkprotocol/ark-ics721/blob/ics721-package/contracts/ics721-base/src/lib.rs

Covered by this test: https://github.com/arkprotocol/ark-ics721/blob/ics721-package/packages/ics721/src/testing/integration_tests.rs#L327-L339

Now there is the sg-ics721 contract, only difference is, it uses sg721-base: https://github.com/arkprotocol/ark-ics721/blob/ics721-package/contracts/sg-ics721/src/execute.rs#L7-L22

Covered by this test: https://github.com/arkprotocol/ark-ics721/blob/ics721-package/contracts/sg-ics721/src/testing/integration_tests.rs#L358-L375

humanalgorithm commented 1 year ago

Question, why exactly is there a different repo for sg721 implementation? Seems like we have the base ics721 repo, but if the only difference between sg721 and cw721 implementation of ics721 is just the one line for instantiate message does it warrant an entire other repo?

taitruong commented 1 year ago

Question, why exactly is there a different repo for sg721 implementation? Seems like we have the base ics721 repo, but if the only difference between sg721 and cw721 implementation of ics721 is just the one line for instantiate message does it warrant an entire other repo?

Good point. Was thinking about anyways putting sg-ics721-bridge contract somewhere else. Not sure it should be in this repo, or rather in some other SG managed repo. Just let me know. For now I will move it here.

taitruong commented 1 year ago

@humanalgorithm, sg-ics721-bridge now moved to this repo. Description above updated:

Now there is the sg-ics721-bridge contract, only difference is, it uses sg721-base: https://github.com/arkprotocol/ark-ics721/blob/ics721-package/contracts/sg-ics721-bridge/src/lib.rs#L80-L95

Covered by this test: https://github.com/arkprotocol/ark-ics721/blob/ics721-package/contracts/sg-ics721-bridge/src/testing/integration_tests.rs#L373-L404

taitruong commented 1 year ago

This PR basically adopts design from cw721 and cw721-base. Have a look for traits here: https://github.com/CosmWasm/cw-nfts/blob/main/packages/cw721/src/traits.rs#L11-L18

Only difference here is, traits in ics721 package have default implementations.

taitruong commented 1 year ago

This PR basically adopts design from cw721 and cw721-base. Have a look for traits here: https://github.com/CosmWasm/cw-nfts/blob/main/packages/cw721/src/traits.rs#L11-L18

Only difference here is, traits in ics721 package have default implementations.

We should consider renaming and follow same conventions as for:

So we may want:

taitruong commented 1 year ago

All tests passed. Except e2e (not covered in CI) - as noted here: https://github.com/public-awesome/ics721/pull/67#discussion_r1312363232 Would be great if some Go expert may look into it. Wasm file for ics721-base is 507kB whilst cw721-base is 307 kB.