Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Escrow 721 setup #5

Merged
merged 34 commits into from
Jun 23, 2022
Merged

Escrow 721 setup #5

merged 34 commits into from
Jun 23, 2022

Conversation

humanalgorithm
Copy link
Contributor

@humanalgorithm humanalgorithm commented Apr 28, 2022

=> Setup working Escrow721 contract
=> setup e2e test in Go to load contracts and test all requisite functions
=> refactoring of e2etest so that we can test each function individually

As part of the ICS721 spec we need an Escrow contract that essentially acts as a container for NFTs. In this PR I am implementing the functions that are needed in an escrow contract. This escrow contract will be used by ICS721 to store NFTs at each connection. For example, if we open a connection between Stargaze and Omni, we would have one of these Escrow contracts instantiated for that channel connection.

Per the spec it should implement the following functions:

function SaveClass(
  classId: string,
  classUri: string) {
  // creates a new NFT Class identified by classId
}

function Mint(
  classId: string,
  tokenId: string,
  tokenUri: string,
  receiver: string) {
  // creates a new NFT identified by <classId,tokenId>
  // receiver becomes owner of the newly minted NFT
}

function Transfer(
  classId: string,
  tokenId: string,
  receiver: string) {
  // transfers the NFT identified by <classId,tokenId> to receiver
  // receiver becomes new owner of the NFT
}

function Burn(
  classId: string,
  tokenId: string) {
  // destroys the NFT identified by <classId,tokenId>
}

function GetOwner(
  classId: string,
  tokenId: string) {
  // returns current owner of the NFT identified by <classId,tokenId>
}

function GetNFT(
  classId: string,
  tokenId: string) {
  // returns NFT identified by <classId,tokenId>
}

function HasClass(classId: string) {
  // returns true if NFT Class identified by classId already exists;
  // returns false otherwise
}

function GetClass(classId: string) {
  // returns NFT Class identified by classId
}

@humanalgorithm humanalgorithm changed the title Escrow 721 basic setup Escrow 721 setup Apr 28, 2022
@@ -0,0 +1,3 @@
use cw_storage_plus::Map;

pub const CLASS_STORAGE: Map<String, String> = Map::new("class_storage");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's preferable to use &str here than owned strings. Did you run into issues using &str?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment about what these strings represent?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to <&str, String> with a comment

class_uri: String,
) -> Result<Response<Empty>, ContractError> {
CLASS_STORAGE.save(deps.storage, class_id, &class_uri)?;
Ok(Response::default())

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add response attributes for indexers "action", "save_class", "class_id", class_id

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added these attributes

CW721ContractWrapper::default().owner_of(deps, env, class_id, token_id, include_expired)
}

fn nft_info(deps: Deps, class_id: String, token_id: String) -> StdResult<NftInfoResponse<Empty>> {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CW Standard: add query_ prefixes to differentiate query implementation functions.

  • fn query_nft_info
  • fn query_has_class
  • fn query_get_class

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mccallofthewild per the IBC spec this should actually be called "GetNFT"
https://github.com/cosmos/ibc/pull/615/files#diff-254e967536211862be86cdfb12569fbcaa03eaf39672f9fae5ab7b226b57cc8fR152

I would like to stay with the spec names wherever possible
But on the other hand I think anytime a query is performed it would be through the query function so possibly it doesn't need to stay with the spec name

Let me know what you think (I changed it to GetNFT for now which is what I originally intended)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@humanalgorithm GetNFT sounds right.

It may be wise to make the functions public as well. The main purpose for the query naming convention is to make it easy to wrap/extend the contract without losing compatibility with the canonical codebase.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mccallofthewild I'll make the query functions public

@humanalgorithm
Copy link
Contributor Author

@mccallofthewild per the duplicate package comment:

This is just the appearance of a duplicate, but not an actual duplicate => let me explain.
So per this article: rust-lang/cargo#1462
When we are referring a Cargo package as a Github address, what we do is specify the Github link and then Cargo auto crawls the repo to find the desired subfolder

Note that the cw721-ibc repo actually has two packages in it, from the Cargo.toml file in cw721-ibc repo:

[profile.release.package.cw721-ibc]
codegen-units = 1
incremental = false

[profile.release.package.cw721-base-ibc]
codegen-units = 1
incremental = false

A note about Cargo packages:
Here it looks like a duplicate because you see the same Github address twice, however once we turn these folder into Cargo packages, the address for each package will actually be different despite the fact that they are part of the same Github repo

Note that I put out a call for someone to turn cw721-ibc into a Cargo package over on the ics721 working group channel.

@mccallofthewild
Copy link

@mccallofthewild per the duplicate package comment...

@humanalgorithm Gotcha. I deleted the comment when I realized it was pointing at a workspace. I'll take a look at cw721-ibc.

@humanalgorithm humanalgorithm merged commit 0f02380 into main Jun 23, 2022
0xekez added a commit that referenced this pull request Jul 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants