Feat/nft example #91

elizabethengelman · 2024-06-27T14:58:56Z

Deployed contract: https://stellar.expert/explorer/testnet/contract/CDLCRZ6ICRW5KB7RFKKJEJ57JQHXLRS2MIBDZ6B4LAZGLKY2WLAJYYLO

examples/soroban/nft/src/lib.rs

examples/soroban/nft/src/subcontract.rs

examples/soroban/nft/src/nft.rs

Apply suggestions from code review Co-authored-by: Willem Wyndham <willem@wyndham.tech>

Now when I run this: ``` just build && \ stellar contract deploy --wasm target/loam/example_nft.wasm --source alice --network local --alias nft && \ stellar contract invoke --id nft -- --help ``` I get this output: ``` Commands: admin_get Get current admin admin_set Transfer to new admin Should be called in the same transaction as deploying the contract to ensure that a different account try to become admin redeploy Admin can redepoly the contract with given hash. mint Mint a new NFT with the given ID, owner, and metadata transfer Transfer an NFT with the given ID from current_owner to new_owner get_nft Get the NFT with the given ID get_owner Find the owner of the NFT with the given ID get_total_count Get the total count of NFTs get_collection_by_owner Get all of the NFTs owned by the given address nft_init Initialize the NFT contract with the given admin and name ```

chadoh

So clean! I confess I found the Map<u32, ()> pretty surprising; glad I found the comment on it in the previous discussion (it's the same as a Set!).

I pushed up a couple changes, to update the Cargo.lock as happens when you run just build now. And I also deployed the contract locally to check the help info and found that it was absent, so I fixed that. Check my commit messages for more info.

Should we add some tests? We could test that the docs I checked manually. Maybe we could also test some common NFT scenarios. We should assume that people will (blindly) copy-paste this into their own project, so we should make sure it works correctly.

crates/loam-cli/examples/soroban/nft/Cargo.toml

crates/loam-cli/examples/soroban/nft/src/nft.rs

…m-sdk Co-authored-by: Chad Ostrowski <221614+chadoh@users.noreply.github.com>

elizabethengelman · 2024-07-16T15:32:06Z

I pushed up a couple changes, to update the Cargo.lock as happens when you run just build now. And I also deployed the contract locally to check the help info and found that it was absent, so I fixed that. Check my commit messages for more info.

Looks great, thanks for pushing those changes up!

Should we add some tests? We could test that the docs I checked manually. Maybe we could also test some common NFT scenarios. We should assume that people will (blindly) copy-paste this into their own project, so we should make sure it works correctly.

I agree! I can get on testing today.

willemneal · 2024-07-16T15:48:42Z

crates/loam-cli/examples/soroban/nft/src/nft.rs

+    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>,


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.

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

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

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? 🤔

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.

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.

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

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.

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, }

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

chadoh · 2024-07-18T15:43:54Z

crates/loam-cli/examples/soroban/nft/src/nft.rs

+
+impl Default for MyNonFungibleToken {
+    fn default() -> Self {
+        MyNonFungibleToken::new(env().current_contract_address(), Bytes::new(env()))


Wait, we are setting the admin to the contract itself? Doesn't that brick it?

Why even bother implementing Default?

Is it something to do with the &mut self conversation above?

Right now Default is required by the macro

it's a nonsensical default. i don't like it. Will this fix it?

[Macro] Add init decorator so that init methods require that the subcontract state is None #92

chadoh · 2024-07-19T15:15:32Z

crates/loam-cli/examples/soroban/nft/src/nft.rs

+
+impl IsInitable for MyNonFungibleToken {
+    fn nft_init(&self, admin: Address, name: Bytes) {
+        self.admin.require_auth();


Wait, how is self.admin already set? And why allow admin to be passed in here, if it's already set? Should admin even be part of the nft_init params?

Having two different kinds of admin—"the one that updates the contract source" vs "the one that mints tokens"—doesn't seem useful?

@elizabethengelman here's how @willemneal helped me implement this in EquitX, for now:

no admin attribute on any subcontract other than the Core / Admin one

add a require_auth function to Contract

while implementing other subcontracts, I can then use crate::Contract and call Contract::require_auth()

While the dependencies are sort of circular, this allows one subcontract to make calls into another.

awesome, thanks so much!

and use Contract::require_admin for authing protected fns

crates/loam-cli/examples/soroban/nft/src/test.rs

Co-authored-by: Willem Wyndham <willem@wyndham.tech>

elizabethengelman · 2024-07-26T16:28:21Z

crates/loam-cli/examples/soroban/nft/src/test.rs

+}
+
+#[test]
+fn test_nft() {


While testing with Env, I had trouble getting the tests to pass while running in parallel. ChatGpt told me that the errors could be caused by a "context already borrowed" issue. It kind of feels like the env isn't being reset in between tests, even though I am presumably creating a new env for each test.

These tests do pass when I run them with just one thread:

cargo test --package example-nft --lib -- test --test-threads=1

@willemneal have you experienced anything like this while testing with env?

elizabethengelman · 2024-07-26T16:34:02Z

crates/loam-cli/examples/soroban/nft/src/test.rs

+
+#[test]
+#[should_panic]
+#[ignore = "This should panic, but it's not"]


I expect that these two tests should panic because we are trying to set/get storage state before the contract is initialized. And I though that it would panic because of this

loam/crates/loam-soroban-sdk/src/lib.rs

Line 25 in bc4cf88

/// This function will panic if the environment has not been initialized.

I could use another set of eyes on this in case I'm missing something.

elizabethengelman commented Jun 27, 2024

View reviewed changes

examples/soroban/nft/src/lib.rs Outdated Show resolved Hide resolved

elizabethengelman marked this pull request as ready for review June 27, 2024 22:07

elizabethengelman force-pushed the feat/nft-example branch 2 times, most recently from 12a5817 to 5d3371e Compare June 28, 2024 16:12

elizabethengelman requested a review from willemneal June 28, 2024 16:44