-
Notifications
You must be signed in to change notification settings - Fork 374
Add #[entry_point] macro attribute to generate entry points #701
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
Conversation
9833ae2
to
7a39f70
Compare
Nice, I will review Monday. My first comment is that In cosmwasm-plus we sometimes import code from one contract to use in another. And to do so, we also gate entry points on a library feature flag as well. See example here https://github.com/CosmWasm/cosmwasm-plus/blob/master/contracts/cw20-base/src/lib.rs#L12 Can you either use the same feature flags as that example, or allow the app developer to do so... the init function should always be present, but the export suppressed if we set the library flag |
Nice we can use the same entry point macro on all the different functions. This does make it cleaner. |
The #[cfg_attr(not(feature = "library"), entry_point)]
pub fn init(
mut deps: DepsMut,
_env: Env,
_info: MessageInfo,
msg: InitMsg,
) -> StdResult<InitResponse> { I tested this here: CosmWasm/cw-plus#216. The wasm32 check remains internal. I.e. |
fdba57e
to
81529d6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added #704 to provide a demo solution to my cosmwasm-plus question. If you have a better solution (eg. #[entry_point]
and #[entry_point(library)]
or always using the library feature flag in the generated macro, I am happy for that as well.
But let's document how to do this properly for cosmwasm-plus.
NB: I tried #[cfg_attr(not(feature = "bibliothek"), entry_point)]
on a crate with the library feature (and no bibliothek feature) and it compiled fine and created the entry point. Maybe just use this in the macro code:
r##"
#[cfg(all(target_arch = "wasm32", not(feature = "library")))]
mod __wasm_export_{name} {{ // new module to avoid conflict of function name
And just document that you can use a "library" feature (if added to the contract's Cargo.toml) to disable entry point generation in wasm
@@ -170,6 +173,36 @@ fn push_and_reduce() { | |||
assert_eq!(counters, vec![(40, 85), (15, 125), (85, 0), (-10, 140)]); | |||
} | |||
|
|||
#[test] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice way to check the entry points
# Needed for testing docs | ||
# "What's even more fun, Cargo packages actually can have cyclic dependencies. | ||
# "(a package can have an indirect dev-dependency on itself)" | ||
# https://users.rust-lang.org/t/does-cargo-support-cyclic-dependencies/35666/3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess if there wasn't such super power, then you could just simplify the doc tests. But cool this works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing we can always do is not not compile an example in the documentation tests, like here:
/// # Example
///
/// ```ignore
/// use contract; // The contract module
///
/// cosmwasm_std::create_entry_points!(contract);
/// ```
However, this disables automatic doc testing for the block and makes it easy to forget when updating code.
packages/derive/src/lib.rs
Outdated
// E.g. "ptr0: u32, ptr1: u32, ptr2: u32," | ||
let typed_ptrs = (0..args).fold(String::new(), |acc, i| format!("{}ptr{}: u32,", acc, i)); | ||
// E.g. "ptr0, ptr1, ptr2," | ||
let ptrs = (0..args).fold(String::new(), |acc, i| format!("{}ptr{},", acc, i)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is ptr0,ptr1,ptr2
without spaces. I would add a space after the comma (in both) or change the comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well spotted. I did not see this because the code is automatically formatted when using cargo expand.
mod __wasm_export_{name} {{ // new module to avoid conflict of function name | ||
#[no_mangle] | ||
extern "C" fn {name}({typed_ptrs}) -> u32 {{ | ||
cosmwasm_std::do_{name}(&super::{name}, {ptrs}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at
cosmwasm/packages/std/src/exports.rs
Lines 96 to 115 in b38e790
pub fn do_handle<M, C, E>( | |
handle_fn: &dyn Fn(DepsMut, Env, MessageInfo, M) -> Result<HandleResponse<C>, E>, | |
env_ptr: u32, | |
info_ptr: u32, | |
msg_ptr: u32, | |
) -> u32 | |
where | |
M: DeserializeOwned + JsonSchema, | |
C: Serialize + Clone + fmt::Debug + PartialEq + JsonSchema, | |
E: ToString, | |
{ | |
let res = _do_handle( | |
handle_fn, | |
env_ptr as *mut Region, | |
info_ptr as *mut Region, | |
msg_ptr as *mut Region, | |
); | |
let v = to_vec(&res).unwrap(); | |
release_buffer(v) as u32 | |
} |
We have the concrete type info from the function we are decorating, the rest is mostly boilerplate.
(I would do that as a separate PR however)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jupp, much to explore from here.
I also though about Multi-value regions in #602, such that we can have always have one pointer input and one pointer output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also though about Multi-value regions in #602, such that we can have always have one pointer input and one pointer output.
I think that would actually make the auto-generation harder, as you are accepting a variable number of input slices and then mapping them to a fixed number of typed variables.
But yeah, plenty of room to explore here :)
Huh, i just saw this comment: #701 (comment) after my other PR. Everytime I click on notifications for the diffs, It just showed me "files are missing" and not the new comments (which I missed). Thanks for that, and let's keep working on this entry_point logic in std, knowing that is can used in cosmwasm-plus |
Demo feature-flagging entry points as per cosmwasm-plus
From my end, happy to see it merged, and another pr or two to extend it. But the foundation is there and looks nice |
Thanks. I think so too. Let me just add the missing spaces of the ptr arguments and then merge. |
Closes #698
Generated code before
via
cosmwasm_std::create_entry_points!(contract)
:Generated code after
Via
#[entry_point]
: