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

Removing boilerplate from token_standards #422

Closed
matklad opened this issue May 19, 2021 · 14 comments · Fixed by #1042
Closed

Removing boilerplate from token_standards #422

matklad opened this issue May 19, 2021 · 14 comments · Fixed by #1042

Comments

@matklad
Copy link
Contributor

matklad commented May 19, 2021

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

#[near_bindgen]
#[derive(BorshDeserialize, BorshSerialize, PanicOnDefault)]
pub struct Contract {
token: FungibleToken,
metadata: LazyOption<FungibleTokenMetadata>,
}
const DATA_IMAGE_SVG_NEAR_ICON: &str = "data:image/svg+xml,%3Csvg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 288 288'%3E%3Cg id='l' data-name='l'%3E%3Cpath d='M187.58,79.81l-30.1,44.69a3.2,3.2,0,0,0,4.75,4.2L191.86,103a1.2,1.2,0,0,1,2,.91v80.46a1.2,1.2,0,0,1-2.12.77L102.18,77.93A15.35,15.35,0,0,0,90.47,72.5H87.34A15.34,15.34,0,0,0,72,87.84V201.16A15.34,15.34,0,0,0,87.34,216.5h0a15.35,15.35,0,0,0,13.08-7.31l30.1-44.69a3.2,3.2,0,0,0-4.75-4.2L96.14,186a1.2,1.2,0,0,1-2-.91V104.61a1.2,1.2,0,0,1,2.12-.77l89.55,107.23a15.35,15.35,0,0,0,11.71,5.43h3.13A15.34,15.34,0,0,0,216,201.16V87.84A15.34,15.34,0,0,0,200.66,72.5h0A15.35,15.35,0,0,0,187.58,79.81Z'/%3E%3C/g%3E%3C/svg%3E";
#[derive(BorshSerialize, BorshStorageKey)]
enum StorageKey {
FungibleToken,
Metadata,
}
#[near_bindgen]
impl Contract {
/// Initializes the contract with the given total supply owned by the given `owner_id` with
/// default metadata (for example purposes only).
#[init]
pub fn new_default_meta(owner_id: ValidAccountId, total_supply: U128) -> Self {
Self::new(
owner_id,
total_supply,
FungibleTokenMetadata {
spec: FT_METADATA_SPEC.to_string(),
name: "Example NEAR fungible token".to_string(),
symbol: "EXAMPLE".to_string(),
icon: Some(DATA_IMAGE_SVG_NEAR_ICON.to_string()),
reference: None,
reference_hash: None,
decimals: 24,
},
)
}
/// Initializes the contract with the given total supply owned by the given `owner_id` with
/// the given fungible token metadata.
#[init]
pub fn new(
owner_id: ValidAccountId,
total_supply: U128,
metadata: FungibleTokenMetadata,
) -> Self {
assert!(!env::state_exists(), "Already initialized");
metadata.assert_valid();
let mut this = Self {
token: FungibleToken::new(StorageKey::FungibleToken),
metadata: LazyOption::new(StorageKey::Metadata, Some(&metadata)),
};
this.token.internal_register_account(owner_id.as_ref());
this.token.internal_deposit(owner_id.as_ref(), total_supply.into());
this
}
fn on_account_closed(&mut self, account_id: AccountId, balance: Balance) {
log!("Closed @{} with {}", account_id, balance);
}
fn on_tokens_burned(&mut self, account_id: AccountId, amount: Balance) {
log!("Account @{} burned {}", account_id, amount);
}
}
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()
}
}

I see two problems with it:

  • its unclear what you should write, you can't auto-complete your way to victory
  • its very opaque. No amount of Rust knowledge can tell you what near_contract_standards::impl_fungible_token_core!(Contract, token, on_tokens_burned); line does.

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

  • just hard-code the known contracts, so that, if it sees impl FungibleToken for, it knows that FungibleTokenCore impl for this type exists, and emits the relevant extern "C" methods

  • for extensibility by third parties, allow for explict extension blocks:

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 austinabell changed the title Removing boilerplaet from token_standards Removing boilerplate from token_standards May 19, 2021
@austinabell
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

matklad commented Jan 19, 2022

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
Copy link
Contributor

austinabell commented Jan 19, 2022

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
Copy link
Contributor

austinabell commented Jan 19, 2022

Experimented with this, see https://github.com/austinabell/standards-experiment (austinabell/standards-experiment@ce623ce). 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
Copy link
Contributor Author

matklad commented Jan 20, 2022

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
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Collaborator

frol commented Jun 28, 2023

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
Copy link
Collaborator

ruseinov commented Jun 28, 2023

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
Copy link
Collaborator

frol commented Jun 30, 2023

@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
Copy link
Collaborator

@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
Copy link
Collaborator

frol commented Jun 30, 2023

@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
Copy link
Collaborator

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants