near / near-sdk-rs

Rust library for writing NEAR smart contracts
https://near-sdk.io
Apache License 2.0
453 stars 241 forks source link

Removing boilerplate from token_standards #422

Closed matklad closed 1 year ago

matklad commented 3 years ago

We currently need a bunch of boilerplate when dealing with token standards:

https://github.com/near/near-sdk-rs/blob/91beb671c242af2f19433b0d4bfe888037f9fbbe/examples/fungible-token/ft/src/lib.rs#L31-L103

I see two problems with it:

I think the ideal interface for this feature would be:

#[near_bindgen]
#[derive(BorshDeserialize, BorshSerialize, PanicOnDefault)]
pub struct Contract {
    ft: FungibleTokenState,
} 

#[near_bindgen]
impl FungibleToken for Contract {
    fn on_account_closed(&mut self, account_id: AccountId, balance: Balance) {
        log!("Closed @{} with {}", account_id, balance);
    }
}

#[near_bindgen]
impl FungibleTokenMetadata for Contract {
    const NAME: &'static str = "Example NEAR fungible token";
    const DECIMALS: u8 = 24;
    ...
} 

Using traits is beneficial, as IDEs can fill in the impls, and it's somewhat more transparent what's going on (as on_account_closed is in an impl for a trait, we see that this is some kind of inversion of control thing).

Now, the problem here is that macros don't know about the traits. Even if we have:

impl<T: FungibleToken> FungibleTokenCore for T {
    fn ft_transfer(&mut self, receiver_id: ValidAccountId, amount: U128, memo: Option<String>) { ... }
}

in the sdk library, near_bindgen doesn't see it, and can't produce the corresponding extern "C" fn ft_transfer. I don't think there's a nice solution here. What I think we could do is

impl MatkladsFungibleToken for Contract { ... } 

#[near_bindgen::extensions]
impl Contract {
    // the user has to repeat the signature here specifcally for near_bindgen.
    // this is the bit we hardcode-away for known contracts
    fn ft_transfer(&mut self, receiver_id: ValidAccountId, amount: U128, memo: Option<String>); // note semicolon
} 

Sidenote: for callbacks, I often like did_do_something / will_do_something naming useful, as it makes it clear when the callback is invoked, before or after the event specified. on_something is more ambiguous. So, did_close_account rather than on_account_closed. (pattern stolen from VS Code APIs)

austinabell commented 3 years ago

I'm a little confused about the proposed example you give. Are you suggesting we code the standards logic into near_bindgen or have it generate the extern "C" interfaces in some generic way?

You definitely know better about what is more composable and feasible, but would it not be possible to have a derive do roughly something like:

#[near_bindgen]  
#[derive(BorshDeserialize, BorshSerialize, PanicOnDefault, FungibleToken)]  
pub struct Contract { 
    #[fungible_token(did_close_account = <some_function>)]
    token: FungibleToken,      
    #[fungible_token(metadata = "get().unwrap()")]
    metadata: LazyOption<FungibleTokenMetadata>,  
} 

which previously was handled by:

#[near_bindgen]
#[derive(BorshDeserialize, BorshSerialize, PanicOnDefault)]
pub struct Contract {
    token: FungibleToken,
    metadata: LazyOption<FungibleTokenMetadata>,
}

near_contract_standards::impl_fungible_token_core!(Contract, token, on_tokens_burned);
near_contract_standards::impl_fungible_token_storage!(Contract, token, on_account_closed);

#[near_bindgen]
impl FungibleTokenMetadataProvider for Contract {
    fn ft_metadata(&self) -> FungibleTokenMetadata {
        self.metadata.get().unwrap()
    }
}
mattlockyer commented 3 years ago

FWIW I posted this in Discord #standards and just wanted to cross post here for visibility of what it's like for a "Mad Scientist of Smart Contracts" (self proclaimed)


Quick high level comment regarding the current sdk standards, specifically the macros implementations.

While they are excellent and really well written code, I find myself having to pull apart the macros and hunt for implementations, just to tweak a few small things.

Example of "token types" where I share TokenMetadata across a "series" of NFTs (editions). https://github.com/near-apps/nft-series/blob/main/contract/src/lib.rs

Notice that I had to pull a lot of the implementations straight from the standard, copy paste, and edit only the slightest of things. Basically, I needed all enumeration methods to go through my custom nft_token method.

Would be cool if things like macros or SDK implementations would allow the passing of custom functions that handle basic things like returning the token based on the token_id AFAIK this is implemented a couple of different ways inside the nft-standard itself.

Just some comments to ponder as we iterate on these standards.

Thanks for all your hard work!

matklad commented 2 years ago

Now, the problem here is that macros don't know about the traits. [near_bindgen] can't produce the corresponding extern "C" fn ft_transfer. I don't think there's a nice solution here.

I think I was partially wrong! There's a solution here, though, yeah, beauty is in the eye of the beholder. The idea is to place actual extern "C" definitions in the upstream crate which defines a token standard, and use the linker trick (the same we already use for unit-testing!) to plug downstream definition into that.

Here's what I am talking about:

The crate which defines the standard (ft-api):

// Rust-side Contract API -- standard Rust trait&Type
pub type Addr = [u32; 4];

pub trait Contract: Send + Sync {
    fn transfer(&self, from: Addr, to: Addr, amount: u64);
    fn ballance(&self, of: Addr) -> u64;
}

// Blockchain-side API *and* implementation of the contract. Physically, "API"
// is some `(func (export "name"))` in WASM, which is represented as `extern
// "C"` in Rust.
//
// Note that this is an actual **implementation** of the said API -- these are
// functions declared & defined in the upstream crate, which use "dynamic"
// dispatch internally to call downstream code
extern "C" fn transfer(from: Addr, to: Addr, amount: u64) {
    unsafe { ft_get_vtable().transfer(from, to, amount) }
}

extern "C" fn ballance(of: Addr) -> u64 {
    unsafe { ft_get_vtable().ballance(of) }
}

// This is the hook which is defined upstream, but whose implementation is going
// to be defined downstream.
extern "C" {
    fn ft_get_vtable() -> &'static dyn Contract;
}

// This allows the user to register their contract as the implementation for the
// singleton instance.
//
// The salient point here is that method signatures don't have to be specified,
// they are encoded solely by the trait.
#[macro_export]
macro_rules! _register {
    ($e:expr) => {
        #[no_mangle]
        extern "C" fn ft_get_vtable() -> &'static dyn $crate::Contract {
            static C: &dyn $crate::Contract = &$e;
            C
        }
    };
}

Standard implementing crate (my-token):

// The user-facing side of thing. `ft_api` can be considered a separate crate

struct MyContract;

impl ft_api::Contract for MyContract {
    fn transfer(&self, from: ft_api::Addr, to: ft_api::Addr, amount: u64) { ... }

    fn ballance(&self, of: ft_api::Addr) -> u64 { ... }
}

ft_api::register!(MyContract);
austinabell commented 2 years ago

I see a few issues with this approach:

  1. If someone does not implement ft_get_vtable (through macro or otherwise) then this will fail at runtime, would be better if this was able to be checked at compile time
  2. If someone only wants to implement part of the API or they want to override the implementation for specific methods, this does not provide the flexibility that a proc macro would allow 3. If someone wanted multiple different implementations of the same standard in one contract (rare case I know) this would not be possible because all would reference the same vtable function

I'm curious to play around with this a bit and see what happens internally with these cases. I'm wondering if the ft_get_vtable gets exported in wasm in this case and also how the implementation of this method can expose the other methods to the wasm binary where they might not otherwise.

Also, I'm curious how this could possibly work safely when a contract does have state. Given the contract is loaded at runtime, I would assume there will have to be more code generation or boilerplate for the API to include these methods.

Final detail which would have to change about this example you give is that the parameters aren't actually included in the extern "C" definitions, as the parameters come from the runtime input host function.

This also doesn't provide a great way to be able to generate and use a trait to call from a contract that does not implement it. Would be ideal if we can provide some API to be able to execute a cross-contract call based on these definitions here.

austinabell commented 2 years ago

Experimented with this, see https://github.com/austinabell/standards-experiment (https://github.com/austinabell/standards-experiment/commit/ce623cea28b9ca6ec76669f21975c78ff6efe321). The issue here is just that when using this pattern, only the vtable function is exposed to wasm (because defined within the contract crate), and none of the others are. Maybe there is a way to trigger the other external functions to be exposed by calling them in a private function generated by the macro?

The issue also is that the defaults for a lot of the standards require using the internal functions of the state (which is usually attached as a field of the contract state) and it might be difficult to express this cleanly given all usages.

My thoughts are maybe we can have another trait (like fn get_tokens(&self) -> &NonFungibleToken with mutable one also possibly) to retrieve the inner struct, and then just have all standards traits be supertraits of this. This might not even work because the lifetimes may be an issue with the default implementations

matklad commented 2 years ago

only the vtable function is exposed to wasm (because defined within the contract crate)

You need #[no_mangle] or #[export_name] which I've forgot

If someone does not implement ft_get_vtable (through macro or otherwise) then this will fail at runtime,

Urgh, right. What I want here is to enforce that ft_get_vtable is statically linked, rather than imported via wasm import. I... don't know how to tell rustc "this one symbol we are going to link statically". for the above example (if I add missing #[no_mangle]s), I get

  (export "ft_get_vtable" (func 3))
  (export "transfer" (func 4))
  (export "ballance" (func 5))

That is, ft_get_table is also callable externally, and that's definitely not something that we want.

If someone only wants to implement part of the API or they want to override the implementation for specific methods

Quite the opposite -- you can leverage usual trait overriding mechanism for that. trait methods can have default impls. Basically, that's the whole idea here -- use standrard Rust tratits as much as possible, instead of inventing a separate copy of OOP via macros.

If someone wanted multiple different implementations of the same standard in one contract

How would that work? My understanding that the whole idea of the standard is that WASM exposes a set of method with specfic names, like ft_transfer. How would you encode two different ft_transfers?

austinabell commented 2 years ago

only the vtable function is exposed to wasm (because defined within the contract crate)

You need #[no_mangle] or #[export_name] which I've forgot

Ah, yes, of course. I missed that detail. This can expose only what we need. The issue is though that we would have to split each standard into its own crate to avoid exposing functions for things not used, or am I missing something?

If someone does not implement ft_get_vtable (through macro or otherwise) then this will fail at runtime,

Urgh, right. What I want here is to enforce that ft_get_vtable is statically linked, rather than imported via wasm import. I... don't know how to tell rustc "this one symbol we are going to link statically". for the above example (if I add missing #[no_mangle]s), I get

  (export "ft_get_vtable" (func 3))
  (export "transfer" (func 4))
  (export "ballance" (func 5))

That is, ft_get_table is also callable externally, and that's definitely not something that we want.

Can just remove the no_mangle from that one, and it seems to work fine, tested with example.

If someone only wants to implement part of the API or they want to override the implementation for specific methods

Quite the opposite -- you can leverage usual trait overriding mechanism for that. trait methods can have default impls. Basically, that's the whole idea here -- use standrard Rust tratits as much as possible, instead of inventing a separate copy of OOP via macros.

Yes, I just mean if someone only wanted to expose some of the methods. I suppose that is a bit of an edge case that shouldn't be considered.

If someone wanted multiple different implementations of the same standard in one contract

How would that work? My understanding that the whole idea of the standard is that WASM exposes a set of method with specfic names, like ft_transfer. How would you encode two different ft_transfers?

Yeah honestly a bit of a lapse on my part, don't know what I was trying to say there

ruseinov commented 1 year ago

I've looked into it and I'd like to understand better what the goal is.

Is the idea to get rid of

near_contract_standards::impl_non_fungible_token_core!(Contract, tokens);
near_contract_standards::impl_non_fungible_token_approval!(Contract, tokens);
near_contract_standards::impl_non_fungible_token_enumeration!(Contract, tokens);

while maintaining the same functionality in terms of near_bindgen?

If we use manual trait implementations for that - we'd have to forgo default implementations as we need access to some of the trait fields. That has it's benefits, as then everything becomes visible and straightforward, but the amount of boilerplate is going to accumulate.

I'd like to understand what the desired interface would be, then it's going to be easy to experiment and produce something acceptable that's doable in rust via proc/derive macro and manual implementations.

What's the vision for extensions? Do they need to complement already existing methods or override them?

frol commented 1 year ago

Is the idea to get rid of ... while maintaining the same functionality in terms of near_bindgen?

Well, the goal is to make it more obvious to the developers what is going on there. Currently, those macros expose contract functions (exposing public interfaces in an implicit way) and implement "hard-coded" logic (see #775 for some evidence that sometimes it is better to give more control to the developers).

If we use manual trait implementations for that - we'd have to forgo default implementations as we need access to some of the trait fields.

There might be some better way, but so far I think it is the most obvious from DevX point of view.

That has it's benefits, as then everything becomes visible and straightforward, but the amount of boilerplate is going to accumulate.

Well, the current boilerplate is impossible to remember magic incantation. Having some trait impl (potentially with some magic if necessary) will at least have some chances of being picked up by IDEs ("implement trait methods" expansion could help) or in the worst case will be copy-pasted from the doc, but maintained and read later as a regular code.

I'd like to understand what the desired interface would be, then it's going to be easy to experiment and produce something acceptable that's doable in rust via proc/derive macro and manual implementations.

There were a number of options explored in the comments above, I am not ready to add anything new to this table.

What's the vision for extensions? Do they need to complement already existing methods or override them?

There is no vision owner at the moment. I am ready to brainstorm the vision together.

ruseinov commented 1 year ago

Well, the current boilerplate is impossible to remember magic incantation. Having some trait impl (potentially with some magic if necessary) will at least have some chances of being picked up by IDEs ("implement trait methods" expansion could help) or in the worst case will be copy-pasted from the doc, but maintained and read later as a regular code.

I agree. Let's start by getting rid of those impl.. macros and looking at the result, iterating from there.

There is no vision owner at the moment. I am ready to brainstorm the vision together.

Let's take it step by step. If there is no vision for extensions yet - I propose we postpone it to see how we can attach those to the implementation logic once that is done. There are plenty of different options, depending on the desired outcome.

frol commented 1 year ago

@ruseinov Regarding future extensions, I just wanted to share this suggestion, so you may use that as a forcing function when thinking about potential future extensions people might want to implement.

ruseinov commented 1 year ago

@ruseinov Regarding future extensions, I just wanted to share this suggestion, so you may use that as a forcing function when thinking about potential future extensions people might want to implement.

Right, that's helpful.

Since the extensions are not currently supported - I propose introducing a separate actionable item for this, because getting of the macros and manually implementing traits is a backwards-compatible and self-isolated change.

frol commented 1 year ago

@ruseinov One more useful case-study is social-db contract, where Storage Management standard is implemented using the trait just fine (worth digging in if we can reduce the boilerplate).

Since the extensions are not currently supported - I propose introducing a separate actionable item for this, because getting of the macros and manually implementing traits is a backwards-compatible and self-isolated change.

Go for it!

ruseinov commented 1 year ago

@ruseinov One more useful case-study is social-db contract, where Storage Management standard is implemented using the trait just fine (worth digging in if we can reduce the boilerplate).

I completely agree that implementing declarative macros just for the sake of saving a line of code per method is not a great solution. It prevents users from customising the implementation while also making it harder to understand.