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

[x/programs] Add NFT program example #511

Closed
wants to merge 1 commit into from
Closed

[x/programs] Add NFT program example #511

wants to merge 1 commit into from

Conversation

exdx
Copy link
Contributor

@exdx exdx commented Oct 3, 2023

Adds a WIP example of a basic ERC-721 program. Support for the standard NFT metadata is included.

This implementation is just an example and will be subject to change.

x/programs/rust/examples/nft/src/lib.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/nft/Cargo.toml Outdated Show resolved Hide resolved
x/programs/rust/examples/nft/src/lib.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/nft/src/lib.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/nft/src/lib.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/nft/src/metadata.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/nft/src/metadata.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/nft/src/metadata.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/nft/src/metadata.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/nft/src/metadata.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@hexfusion hexfusion left a comment

Choose a reason for hiding this comment

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

Looking good a few notes. As I noted offline I think implementing this as an example/test in go will answer my other questions.

x/programs/rust/examples/nft/src/lib.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/nft/src/metadata.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/nft/src/lib.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/nft/src/lib.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/nft/src/lib.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/nft/src/lib.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/nft/src/lib.rs Outdated Show resolved Hide resolved
@exdx exdx force-pushed the token/nft branch 2 times, most recently from 3eea9c6 to e56de4d Compare October 6, 2023 17:59
@exdx exdx marked this pull request as ready for review October 6, 2023 20:04
x/programs/examples/nft_test.go Outdated Show resolved Hide resolved
@patrick-ogrady
Copy link
Contributor

GOAL: Can we implement this with just Rust/YAML once simulator is merged?

@exdx
Copy link
Contributor Author

exdx commented Oct 17, 2023

GOAL: Can we implement this with just Rust/YAML once simulator is merged?

Sounds good. Just waiting for #567 to land first.

@exdx exdx force-pushed the token/nft branch 2 times, most recently from c3a03f6 to e158dda Compare October 18, 2023 15:05
Copy link
Contributor

@richardpringle richardpringle left a comment

Choose a reason for hiding this comment

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

Major comment is that there doesn't seem to be anything non-fungible about the NFT program. Maybe I'm missing something though

x/programs/rust/examples/nft/src/example.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/nft/src/example.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/nft/src/example.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/nft/src/example.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/nft/src/example.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/nft/src/lib.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/nft/src/lib.rs Show resolved Hide resolved
x/programs/rust/examples/nft/src/lib.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/nft/src/lib.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/nft/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@samliok samliok left a comment

Choose a reason for hiding this comment

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

looks good, just some q's 👍

x/programs/rust/examples/nft/src/lib.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/nft/src/lib.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/nft/src/lib.rs Outdated Show resolved Hide resolved
@exdx
Copy link
Contributor Author

exdx commented Oct 19, 2023

Updated the PR per the latest round of reviews.

#537 will land and then we can hook everything up for testing. Both the program and the test are now 100% in Rust.

@exdx
Copy link
Contributor Author

exdx commented Oct 20, 2023

Per @patrick-ogrady this program will be an example of a single unique NFT, for example a building or a unique piece of art.
The contract will be renamed to single_nft.

A subsequent NFT program will add support for collections. This NFT program is the first one to land and should remain as simple and readable as possible.

At this point the program is ready to go after testing. I will also add a README.

@exdx exdx requested a review from patrick-ogrady October 27, 2023 15:29
x/programs/rust/examples/nft/tests/nft.rs Show resolved Hide resolved
x/programs/rust/examples/nft/Cargo.toml Outdated Show resolved Hide resolved
x/programs/rust/examples/nft/src/lib.rs Outdated Show resolved Hide resolved
@exdx
Copy link
Contributor Author

exdx commented Oct 31, 2023

Currently waiting on support for String arguments in public functions.
Other followups include

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
wasmlanche_sdk = { version = "0.1.0", path = "../../wasmlanche_sdk", features = ["simulator"]}
Copy link
Contributor

Choose a reason for hiding this comment

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

is simulator a dev-dependency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This feature is tracked in rust-lang/cargo#7916. It's in nightly but not sure if it's in stable.


assert_eq!(counter, 0, "init already called");

// Set token name
Copy link
Contributor

Choose a reason for hiding this comment

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

Everything below this line in this function is a perfect example of what I meant by "batch" ops.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My impression was batch ops are something that the underlying SDK would add support for behind the scenes? I am not aware of changes that should be made to the program to support batching.

"balance must be greater than or equal to amount burned"
);

let counter = program
Copy link
Contributor

@patrick-ogrady patrick-ogrady Oct 31, 2023

Choose a reason for hiding this comment

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

Counter is never updated but you check against it when burning? Is it supposed to be the total count ever created or the current supply?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Counter is the total count of outstanding NFTs. It goes up but does not go down. Basically it's there to ensure that the max supply is never exceeded. Burning an NFT does not decrement the counter because the NFT itself is gone at that point and should not be able to be re-minted.

@exdx exdx force-pushed the token/nft branch 2 times, most recently from 24bb366 to b1e6e8a Compare November 2, 2023 13:40
Signed-off-by: Dan Sover <dan.sover@avalabs.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants