Skip to content

Feat/nft example #91

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.

Sign up for GitHub

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

Open
elizabethengelman wants to merge 31 commits into
base: main
Choose a base branch
from
Open

Feat/nft example #91

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
4849202
Update dev_dependencies -> dev-dependencies in examples
elizabethengelman Jun 26, 2024
8e6d9c9
Add NFT example
elizabethengelman Jun 26, 2024
9cf14ea
Fix build errors
elizabethengelman Jun 27, 2024
e1404c3
Change NFT ids to u32s
elizabethengelman Jun 27, 2024
4f31132
Refactor so that the nft id is generated in mint
elizabethengelman Jun 27, 2024
e647a55
Formatting & clippy
elizabethengelman Jun 28, 2024
a1fb4ff
Fully qualify soroban_sdk types in subcontract traits
elizabethengelman Jul 1, 2024
aabcdfa
Panic on transfer if the NFT id does not exist
elizabethengelman Jul 2, 2024
2c7fefd
Fix owner_id check
elizabethengelman Jul 2, 2024
1bad366
Update examples/soroban/nft/src/nft.rs
elizabethengelman Jul 2, 2024
432c914
Cargo fmt updates
elizabethengelman Jul 2, 2024
abe69e2
Make ft_init fn accept &self instead of &mut self
elizabethengelman Jul 2, 2024
765fbf3
Update examples/soroban/nft/src/nft.rs
elizabethengelman Jul 3, 2024
1aa5b11
Fix owners_to_nft_ids behavior 🙈
elizabethengelman Jul 3, 2024
b46b4c5
Move nft example to loam-cli dir
elizabethengelman Jul 15, 2024
b18d6d6
Use derive_contract for example nft
elizabethengelman Jul 15, 2024
959952a
Refactor owners_to_nft_ids
elizabethengelman Jul 15, 2024
6e41c99
build: 'loam build' changes Cargo.lock
chadoh Jul 15, 2024
95d2161
fix: use doc comments for subcontracts
chadoh Jul 15, 2024
ba7af15
Remove extraneous comments in nft.rs
elizabethengelman Jul 15, 2024
ccaa9ae
Remove loam-soroban-sdk from Cargo.toml as it is re-exported from loa…
elizabethengelman Jul 15, 2024
d3acb1a
Require auth from the admin for minting
elizabethengelman Jul 16, 2024
cc22d8b
Bump total count limit
elizabethengelman Jul 16, 2024
553f401
Remove admin attribute from MyNonFungibleToken
elizabethengelman Jul 19, 2024
ec7df81
WIP: add happy path tests for nft example
elizabethengelman Jul 22, 2024
7e7cfbf
Apply suggestions from code review
elizabethengelman Jul 24, 2024
5655fd8
Cleanup happy path test
elizabethengelman Jul 26, 2024
3d83b38
Add tests for error cases
elizabethengelman Jul 26, 2024
3a6507c
Ensure that panicks are happening for the right reasons
elizabethengelman Jul 26, 2024
63ed1f4
Factor out some commonly use fns in nft test
elizabethengelman Jul 26, 2024
dc9ed7c
Add Metadata type for NFT example
elizabethengelman Jul 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions crates/loam-cli/examples/soroban/nft/src/nft.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ use crate::subcontract::{IsInitable, IsNonFungible};
pub struct MyNonFungibleToken {
admin: Address,
name: Bytes,
total_count: u32,
owners_to_nft_ids: Map<Address, Map<u32, ()>>, // the owner's collection
nft_ids_to_owners: Map<u32, Address>,
nft_ids_to_metadata: Map<u32, Bytes>,
total_count: u128,
owners_to_nft_ids: Map<Address, Map<u128, ()>>, // the owner's collection
nft_ids_to_owners: Map<u128, Address>,
nft_ids_to_metadata: Map<u128, Bytes>,
Copy link
Copy Markdown
Collaborator

@willemneal willemneal Jul 16, 2024

Choose a reason for hiding this comment

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

Hmm it seems annoying that the metadata is untyped. Would be nice for consumers of the contract to know what the shape of the metadata is. But if is the current standard it's fine, but it's something we should push on to get agreement.

Copy link
Copy Markdown
Contributor

@chadoh chadoh Jul 16, 2024

Choose a reason for hiding this comment

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

I assumed that this was an intentional part of the spec, to allow rapid ecosystem experimentation and de facto standards to arise.

Copy link
Copy Markdown
Collaborator

@willemneal willemneal Jul 16, 2024

Choose a reason for hiding this comment

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

Still annoying. You need to know how to deserialize the bytes...

Copy link
Copy Markdown
Contributor Author

@elizabethengelman elizabethengelman Jul 16, 2024

Choose a reason for hiding this comment

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

Yeah, that's a great point. From what I can tell from SEP-0039 best practices are to include a few key fields in the metadata: name, description, URL, issue, and code.

What if we start with those fields in a Metadata struct, and then if/when a standard emerges we can update the type? Not sure how difficult it will be to upgrade subcontracts, though with the ability to redeploy, that seems doable.

pub struct Metadata {
    name: Bytes,
    description: Bytes,
    url: Bytes,
    issuer: Address,
    code: Bytes,
}

alternatively we could have them all be optional fields? 🤔

Copy link
Copy Markdown
Collaborator

@willemneal willemneal Jul 16, 2024

Choose a reason for hiding this comment

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

Personally I think the first three should be required. And Perhaps we use String?. It's again annoying because the all seem like that is what they will be. So perhaps we just put this forward as our standard. Other contracts will convert from Bytes to string anyway so we are just skipping a step.

Copy link
Copy Markdown
Contributor

@chadoh chadoh Jul 16, 2024

Choose a reason for hiding this comment

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

Could we implement it as two separate subcontracts, IsSep39 and IsSep39Extension, or something like that? Implementing IsSep39Extension would override all instances of metadata from Bytes to this suggested Metadata struct.

Copy link
Copy Markdown
Contributor

@chadoh chadoh Jul 16, 2024

Choose a reason for hiding this comment

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

Or IsSep39WithTypedMetadata, or IsTypedMetadata, or... ¯\_(ツ)_/¯

Copy link
Copy Markdown
Contributor Author

@elizabethengelman elizabethengelman Jul 29, 2024

Choose a reason for hiding this comment

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

Finally getting back to addressing this.

I like the idea of creating an IsSep39Metadata subcontract, but I'm unsure how to implement it.

  • One way I tried was to have an IsSep39Metdata subcontract with a Metadata associated type:
impl IsTypedMetadata for MyNonFungibleToken {
    type Metadata = MyNonFungibleTokenMetadata;
}

The issue I ran into with this is that our derive_contract doesn't generate the associate type over to the generated trait TypedMetadata. It just adds the Impl generated type. I'm not sure if this is something we actually want to add to the macro, or if there is a better way to do this, but I moved on and tried a different idea...

  • The other idea was to have IsTypedMetadata include a deserialize_metadata fn that transforms the Bytes metadata into the MyNonFungibleTokenMetadata type. But that is still annoying because the user needs to serialize the metadata into Bytes when minting, etc.

I'll keep thinking on this, but I could probably use another set of eyes on this.

Copy link
Copy Markdown
Contributor Author

@elizabethengelman elizabethengelman Jul 29, 2024

Choose a reason for hiding this comment

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

Also, for now I've just added a Metadata type like the following, maybe we can move forward with that and then continue to improve the standard/make sure that the Metadata can be flexible going forward.

pub struct Metadata {
    pub(crate) name: String,
    pub(crate) description: String,
    pub(crate) url: String,
}

Copy link
Copy Markdown
Collaborator

@willemneal willemneal Jul 31, 2024

Choose a reason for hiding this comment

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

With #[contracttype] the name of the type exported, so perhaps we could just called it Sep39Metadata?

}

impl MyNonFungibleToken {
Expand Down Expand Up @@ -44,7 +44,7 @@ impl IsInitable for MyNonFungibleToken {
}

impl IsNonFungible for MyNonFungibleToken {
fn mint(&mut self, owner: Address, metadata: Bytes) -> u32 {
fn mint(&mut self, owner: Address, metadata: Bytes) -> u128 {
self.admin.require_auth();

let current_count = self.total_count;
Expand All @@ -66,7 +66,7 @@ impl IsNonFungible for MyNonFungibleToken {
new_id
}

fn transfer(&mut self, id: u32, current_owner: Address, new_owner: Address) {
fn transfer(&mut self, id: u128, current_owner: Address, new_owner: Address) {
current_owner.require_auth();
let owner_id = self.nft_ids_to_owners.get(id).expect("NFT does not exist");
assert!(
Expand Down Expand Up @@ -97,19 +97,19 @@ impl IsNonFungible for MyNonFungibleToken {
self.owners_to_nft_ids.set(new_owner, new_owner_collection);
}

fn get_nft(&self, id: u32) -> Option<Bytes> {
fn get_nft(&self, id: u128) -> Option<Bytes> {
self.nft_ids_to_metadata.get(id)
}

fn get_owner(&self, id: u32) -> Option<Address> {
fn get_owner(&self, id: u128) -> Option<Address> {
self.nft_ids_to_owners.get(id)
}

fn get_total_count(&self) -> u32 {
fn get_total_count(&self) -> u128 {
self.total_count
}

fn get_collection_by_owner(&self, owner: Address) -> Vec<u32> {
fn get_collection_by_owner(&self, owner: Address) -> Vec<u128> {
self.owners_to_nft_ids
.get(owner)
.unwrap_or(Map::new(env()))
Expand Down
12 changes: 6 additions & 6 deletions crates/loam-cli/examples/soroban/nft/src/subcontract.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,30 +7,30 @@ pub trait IsNonFungible {
&mut self,
owner: loam_sdk::soroban_sdk::Address,
metadata: loam_sdk::soroban_sdk::Bytes,
) -> u32;
) -> u128;

/// Transfer the NFT with the given ID from current_owner to new_owner
fn transfer(
&mut self,
id: u32,
id: u128,
current_owner: loam_sdk::soroban_sdk::Address,
new_owner: loam_sdk::soroban_sdk::Address,
);

/// Get the NFT with the given ID
fn get_nft(&self, id: u32) -> Option<loam_sdk::soroban_sdk::Bytes>;
fn get_nft(&self, id: u128) -> Option<loam_sdk::soroban_sdk::Bytes>;

/// Find the owner of the NFT with the given ID
fn get_owner(&self, id: u32) -> Option<loam_sdk::soroban_sdk::Address>;
fn get_owner(&self, id: u128) -> Option<loam_sdk::soroban_sdk::Address>;

/// Get the total count of NFTs
fn get_total_count(&self) -> u32;
fn get_total_count(&self) -> u128;

/// Get all of the NFTs owned by the given address
fn get_collection_by_owner(
&self,
owner: loam_sdk::soroban_sdk::Address,
) -> loam_sdk::soroban_sdk::Vec<u32>;
) -> loam_sdk::soroban_sdk::Vec<u128>;
}

#[subcontract]
Expand Down