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

Provide default implementations for token standards #1050

Open
agostbiro opened this issue Jul 9, 2023 · 4 comments
Open

Provide default implementations for token standards #1050

agostbiro opened this issue Jul 9, 2023 · 4 comments

Comments

@agostbiro
Copy link
Contributor

agostbiro commented Jul 9, 2023

We're deprecating declarative macros for token standards (see #1042 and #1049) to favor explicit implementations.

As suggested in #422 (comment), we could provide default implementations for token standard traits for contracts that implement traits to expose their token struct members.

For example, in near-contract-standards:

pub trait NonFungibleTokenContract {
    fn non_fungible_token(&self) -> &NonFungibleToken;
    fn non_fungible_token_mut(&mut self) -> &mut NonFungibleToken;
}

impl<T: NonFungibleTokenContract> NonFungibleTokenCore for T {
    fn nft_transfer(
        &mut self,
        receiver_id: AccountId,
        token_id: TokenId,
        approval_id: Option<u64>,
        memo: Option<String>,
    ) {
        self.non_fungible_token_mut().nft_transfer(receiver_id, token_id, approval_id, memo)
    }

    fn nft_transfer_call(
        &mut self,
        receiver_id: AccountId,
        token_id: TokenId,
        approval_id: Option<u64>,
        memo: Option<String>,
        msg: String,
    ) -> PromiseOrValue<bool> {
        self.non_fungible_token_mut().nft_transfer_call(
            receiver_id,
            token_id,
            approval_id,
            memo,
            msg,
        )
    }

    fn nft_token(&self, token_id: TokenId) -> Option<Token> {
        self.non_fungible_token().nft_token(token_id)
    }
}

impl<T: NonFungibleTokenContract> NonFungibleTokenResolver for T {
    fn nft_resolve_transfer(
        &mut self,
        previous_owner_id: AccountId,
        receiver_id: AccountId,
        token_id: TokenId,
        approved_account_ids: Option<HashMap<AccountId, u64>>,
    ) -> bool {
        self.non_fungible_token_mut().nft_resolve_transfer(
            previous_owner_id,
            receiver_id,
            token_id,
            approved_account_ids,
        )
    }
}

// etc

And then users can just implement NonFungibleTokenContract to get the NFT trait methods:

struct MyContract {
    non_fungible_token: NonFungibleToken,
}

impl NonFungibleTokenContract for MyContract {
    fn non_fungible_token(&self) -> &NonFungibleToken {
        &self.non_fungible_token
    }

    fn non_fungible_token_mut(&mut self) -> &mut NonFungibleToken {
        &mut self.non_fungible_token
    }
}

The examples should be updated as well with this pattern where it makes sense.

@frol
Copy link
Collaborator

frol commented Jul 11, 2023

Great suggestion to implement NonFungibleTokenContract trait, so then we can have default impls for NonFungibleTokenCore! Though, I would avoid blanket implementation. Instead, I propose the following:

Assuming we have the following setup:

struct NonFungibleToken {}
impl NonFungibleToken {
    fn inner_nft_transfer(&self, id: String, receiver: String) {
        println!("INNER NFT TRANSFER {id} to {receiver}");
    }
}

trait NonFungibleTokenContract {
    fn get_non_fungible_token(&self) -> &NonFungibleToken;
}

// This is just the default implementation, but it is not a blanket implementation:
trait NonFungibleTokenCore: NonFungibleTokenContract {
    fn nft_transfer(&self, id: String, receiver: String) {
        self.get_non_fungible_token().inner_nft_transfer(id, receiver);
    }
}

Here is how I would implement my smart contract that needs the default NFT logic:

struct MyContract {
    non_fungible_token: NonFungibleToken,
}

impl NonFungibleTokenContract for MyContract {
    fn get_non_fungible_token(&self) -> &NonFungibleToken {
        &self.non_fungible_token
    }
}

impl NonFungibleTokenCore for MyContract {}

Here is how I would customize one of the methods (just implement it in the trait implementation):

impl NonFungibleTokenCore for MyContract {
    fn nft_transfer(&self, id: String, receiver: String) {
        println!("CUSTOM LOGIC");
    }
}

Here is the playground.

The problem, however, is #[near_bindgen] has no clue about the default methods and thus it cannot generate extern "C" functions. That is:

#[near_bindgen]
impl NonFungibleTokenCore for MyContract {}

, will not generate the exported functions. There is no clear solution in my head at this point. Maybe we can implement "trait registration"?

@frol
Copy link
Collaborator

frol commented Jul 11, 2023

Another consideration point is the function attributes like #[payable]:

/// #[near_bindgen]
/// impl NonFungibleTokenApproval for Contract {
/// #[payable]
/// fn nft_approve(&mut self, token_id: TokenId, account_id: AccountId, msg: Option<String>) -> Option<Promise> {
/// self.tokens.nft_approve(token_id, account_id, msg)
/// }
///
/// #[payable]
/// fn nft_revoke(&mut self, token_id: TokenId, account_id: AccountId) {
/// self.tokens.nft_revoke(token_id, account_id);
/// }
///
/// #[payable]
/// fn nft_revoke_all(&mut self, token_id: TokenId) {
/// self.tokens.nft_revoke_all(token_id);
///
/// }
///
/// fn nft_is_approved(&self, token_id: TokenId, approved_account_id: AccountId, approval_id: Option<u64>) -> bool {
/// self.tokens.nft_is_approved(token_id, approved_account_id, approval_id)
/// }
/// }
. Those also need to be aligned. I feel that at this stage it might only make things harder to reason about in contrast to just offering developers nice copy-pastable snippets in the documentation.

@agostbiro agostbiro removed the good first issue Good for newcomers label Jul 11, 2023
@agostbiro
Copy link
Contributor Author

Thanks @frol for raising great points! I jumped the gun on the example.

I agree with avoiding the blanket implementations in favor of the solution that you proposed.

I don't have a good solution either for generating extern "C" functions, but I feel that if we find a nice solution there, we could just add the payable attributes in the default implementation. But I also understand your concern that we might end up adding too much complexity with this feature.

Should we leave this open for a weeks to see if we come up with something simple for the extern generation problem and if not just close it?

@frol
Copy link
Collaborator

frol commented Jul 30, 2023

Should we leave this open for a weeks to see if we come up with something simple for the extern generation problem and if not just close it?

I think we can keep it open similarly to #1056. Probably we need to introduce a new label for such dead-end issues, so we can close them and easily discover them in the future.

@frol frol added the P2 label Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants