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

Define a macro for extending CW721 execute/query messages #90

Open
larry0x opened this issue Oct 21, 2022 · 3 comments
Open

Define a macro for extending CW721 execute/query messages #90

larry0x opened this issue Oct 21, 2022 · 3 comments

Comments

@larry0x
Copy link
Collaborator

larry0x commented Oct 21, 2022

The problem

cw721::ExecuteMsg defines a minimum set of execute methods that all CW-721 variants are expected to implement. However, projects often want to extend it by adding their own methods. For example, cw721-base adds "mint" and "burn" methods. The process for adding these methods is rather cumbersome, for example:

// cw721-base/src/msg.rs
pub enum Cw721ExecuteMsg {
    // below are all copy-pasted from cw721::ExecuteMsg
    TransferNft { .. },
    SendNft { .. },
    Approve { .. },
    Revoke { .. },
    ApproveAll { .. },

    // only the following are new methods added by cw721-base
    RevokeAll { .. },
    Burn { .. }
}

Imo, there are two drawbacks with this approach:

  • A lot of copy-pasting is needed (very verbose)
  • It is not enforced that the execute message implements all variants required by CW721 specs (What if, say, TransferNft is omitted by mistake? Preferably we want an error to be thrown at compile time to alert the developer that they forgot something.)

Proposed solution

@0xekez showed me it is possible to "autofill" an enum with some pre-defined variants by a macro, which is an approach already used by DAO DAO: https://github.com/DA0-DA0/dao-contracts/blob/main/packages/cwd-macros/src/lib.rs

I propose we create a #[cw721_execute] macro that if used the following way:

use cw721::cw721_execute;

#[cw721_execute]
enum ExecuteMsg {
    Foo,
    Bar,
}

Will transform the enum to:

#[cw_serde]
enum ExecuteMsg {
    TransferNft { .. },
    SendNft { .. },
    Approve { .. },
    Revoke { .. },
    ApproveAll { .. },
    Foo,
    Bar,
}

This prevents the developer to have to copy all the CW721 enum variants over, and enforces all variants are included. Additionally, #[cw_serde] is automatically applied.

In the cw721 crate, the execute message can simply be defined as:

#[cw721_execute]
pub enum ExecuteMsg {}

Further suggestion

It'd be even better if the following traits can be automatically implemented as well:

impl From<cw721::ExecuteMsg> for ExecuteMsg {
    fn from(msg: cw721::ExecuteMsg) -> Self {
        match msg {
            cw721::ExecuteMsg::TransferNft { .. } => Self::TransferNft { .. },
            // ...
        }
    }
}

impl TryFrom<ExecuteMsg> for cw721::ExecuteMsg {
    type Error = String; // or other error type we prefer
    fn try_from(msg: ExecuteMsg) -> Result<Self, Self::Error> {
        match msg {
            ExecuteMsg::TransferNft { .. } => Ok(cw721::ExecuteMsg::TransferNft { .. }),
            // ...
            ExecuteMsg::Foo => Err("`cw721::ExecuteMsg` does not have variant `Foo`".to_string()),
            ExecuteMsg::Bar => Err("`cw721::ExecuteMsg` does not have variant `Bar`".to_string()),
        }
    }
}

This makes it much easier to implement execute entry point:

#[entry_point]
pub fn execute(
    deps: DepsMut,
    env: Env,
    info: MessageInfo,
    msg: ExecuteMsg,
) -> Result<Response, ContractError> {
    match msg {
        // For custom execute methods, dispatch to the appropriate
        // handler functions
        ExecuteMsg::Foo => execute_foo(deps, env, info),
        ExecuteMsg::Bar => execute_bar(deps, env, info),

        // For generic cw721 execute methods,
        // instead of writing a bunch of matching statements,
        // simply cast `ExecuteMsg` into the parent message type
        // and dispatch to the parent contract.
        msg => {
            let parent = Cw721Contract::<Empty, Empty>::default();
            parent.execute(deps, env, info, msg.try_into()?)
        },
    }
}

Notes

  • If adopted, this change will deprecate cw721::ExecuteMsg::Extension method as it is no longer needed.
@JakeHartnell
Copy link
Collaborator

JakeHartnell commented Oct 21, 2022

Love this. ❤️

Everyone hates the extension pattern, would allow us to simplify a lot of stuff. 😂

@shanev
Copy link
Collaborator

shanev commented Oct 21, 2022

This greatly simplifies things for the developer that knows what's going on under the hood. But what about a noob dev that doesn't?

@webmaster128
Copy link
Member

Great to see this solution by @0xekez / the DAO DAO team. Some thoughts:

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

Successfully merging a pull request may close this issue.

4 participants