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

Add an API to provide additional attributes to be added to generated entities #379

Closed
hashedone opened this issue May 9, 2024 · 6 comments · Fixed by #388
Closed

Add an API to provide additional attributes to be added to generated entities #379

hashedone opened this issue May 9, 2024 · 6 comments · Fixed by #388
Assignees
Milestone

Comments

@hashedone
Copy link
Collaborator

Due to #374, there might be a need to add attributes to generated entities - here, in particular, derive macros on the generated messages.
The design for that is as discussed:

#[interface]
#[sv::attr(msg(exec), derive(cw_orch::ExecuteFns))]
trait MyInterface {
}

That approach gives us flexibility. It is useful for many cases that we might not even know about yet.

The sv::attr attribute is defined as #[sv::attr(target, attribute)]. target defines what the attribute is forwarded to, and attribute is the attribute itself.

For now, we would forward the attributes only for messages, as this is part of the generated code aligned with the non-Sylvia approach, so there is a high chance that custom attributes for them would be useful. In the future, we could add another target, e.g. entry points, remotes, helpers, etc.

The target right now would be a msg(msg_type) adding the attribute to the message of the type:

  • exec for the execute message
  • query for the query message
  • instantiate for the instantiate message

On top of that, msg should support multiple message types, like msg(exec, query), or skip the message types, in which case the entity should be generated for all the messages.

@hashedone hashedone added this to the 1.1.0 milestone May 9, 2024
@hashedone
Copy link
Collaborator Author

I am even considering extending this further:

#[sv::attr(msg, attribute1, attribute2, ...)]

This way it would be more compact to add more attributes at once. Without that it might get complex when we would need separate set of attributes for different messages, eg:

#[sv::attr(msg(exec), attrexec1)]
#[sv::attr(msg(exec), attrexec2)]
#[sv::attr(msg(query), attrquyer1)]
#[sv::attr(msg(query), attrquery2)]

Adding the possibility to flatten attributes would make this slightly less boilerplaty.

@ethanfrey
Copy link
Member

I agree with the idea. However, I was playing with my fork to see if I could integrate sylvia and cw-orch both in mesh security and came across an issue: cw-orch requires custom tags on the variants that accept payment. It looks something like this:

#[cw_serde]
#[derive(cw_orch::ExecuteFns)]
pub enum ExecuteMsg {
    Timeout {}
    #[payable]
    Create {},
}

For good UX, only some of the fields should have this. To enable this to work at all, we could add #[payable] everywhere. Neither is possible right now, nor with this proposal I believe. It will allow adding #[derive(cw_orch::ExecuteFns)] but not the extra #[payable] tag on some variants
`

@jawoznia
Copy link
Collaborator

jawoznia commented Jun 3, 2024

We could also provide a way to forward attributes to message variants.

#[contract]
#[sv::derive_attr(msg(exec), attribute1, attribute2, ...)]
impl Contract {
    #[sv::msg(exec, attr(attr1, attr2 ...))]
    pub fn exec(...) -> ... {}
}

And get output like

#[cw_serde]
#[derive(attribute1, attribute2)]
pub enum ExecMsg {
    #[attr1]
    #[attr2]
    Exec {},
}

#[cw_serde]
#[derive(attribute1, attribute2)]  // Do we want/have to forward those attrs to Wrapper message too?
pub enum ContractExecMsg {
    Contract(ExecMsg)
}

@ethanfrey
Copy link
Member

It looks good, adding a way to pass attributes to message variants.

however, can you update the example to show the correct embedding? I assume it should be ContractExecMsg::Exec(ExecMsg) right now they are independent and not wrapping

@ethanfrey
Copy link
Member

PS do you use serde(untagged) internally?

in any case, I think we need input from the abstract team on this exact use case, I am not an expert on the formatting. But support the ability to use both together

@jawoznia
Copy link
Collaborator

jawoznia commented Jun 3, 2024

PS do you use serde(untagged) internally?

in any case, I think we need input from the abstract team on this exact use case, I am not an expert on the formatting. But support the ability to use both together

Yes, #[serde(untagged)] is always added to the wrapper message signature.

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