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

feat: refactor ext API and add new NEP264 functionality #742

Merged
merged 20 commits into from
May 12, 2022

Conversation

austinabell
Copy link
Contributor

@austinabell austinabell commented Mar 4, 2022

closes #740

child PR to near/nearcore#6285 (when/if it comes in)

Few interconnected changes so I will list them here:

  • Add new syscall, env, and Promise API function for NEP addition
  • changed ext API to auto-generate bindings to call function, not relying on duplicate definitions
    • generates a builder pattern <ContractName>Ext that allows configuring transaction metadata like gas and deposit, and generates the codegen for every callable function which consumes the builder
    • This is not exposed to the wasm binary so no cost unless used
  • Change any struct generated to serialize params to use references to everything to avoid moving and make things consistent (only need a reference to serialize)
  • Removes some duplication within macros and general cleanup
    - TODO: Update existing ext_contract API to use new host function/pattern

The call will not succeed to workspaces tests will fail because sandbox isn't updated to have this host function, but we can at least iterate on the API.

Things that I'd like input on:

  • should attaching value to a transaction be with with_amount, with_deposit, or some other function name? field on Ext struct will be updated with this to match
  • Should we create a newtype for the weight? Currently just a u64 for ergonomics, but can create a wrapper type to avoid misuse
  • Should the Ext struct be clonable? Might simplify some patterns where a common configuration is used, but also might open the API up to potential misuse

@austinabell austinabell changed the title Add new host function to sys and env feat: refactor ext API and add new NEP264 functionality Mar 4, 2022
Copy link
Contributor

@itegulov itegulov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a huge improvement, nice!

Just a bunch of nitpicks for now, I need to make another in-depth pass

near-sdk-macros/src/core_impl/code_generator/serializer.rs Outdated Show resolved Hide resolved
if let Some(generics) = generic_details {
// If ext generation is on struct, make ext function associated with struct not module
ext_code = quote! {
impl#generics #ident#generics {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

syn::Generics include where clause in it, right? Would be cool to have a test for that.

near-sdk-macros/src/core_impl/code_generator/ext.rs Outdated Show resolved Hide resolved
near-sdk-macros/src/core_impl/code_generator/ext.rs Outdated Show resolved Hide resolved
near-sdk/src/promise.rs Outdated Show resolved Hide resolved
ext::merge_sort(arr0, account_id.clone(), 0, prepaid_gas / 4)
.and(ext::merge_sort(arr1, account_id.clone(), 0, prepaid_gas / 4))
.then(ext::merge(account_id, 0, prepaid_gas / 4))
Self::ext(account_id.clone())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't account_id always be env::current_account_id() here since we are calling the current contract?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, it is. This is just cloning that every time to avoid using the host function three times (unnecessary gas usage)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well then perhaps the Self could handle this making the API a little more ergonomic and preventing users from calling another contract.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But they can call the self API to another contract. I thought you meant only in this case. The account_id can absolutely be different than the current.

I don't think it's worth adding another method for calling it on self, because cases like this you might want to avoid calling the host function more than once

Copy link
Contributor

@willemneal willemneal Mar 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another contract might have the same interface as the current contract, but I would personally propose Self always be calling the current contract. It would make reading over the contract easier:

Self::merge_sort(arr0).and(Self::merge_sort(arr1)).then(Self::merge())

And my point with Self handling the account_id is that it could lazily load and cache it.

Also You could still have Self::ext if you need the functionality, but for most uses of Self this would be a better DevX.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't work. These methods already exist. Feel free to create a commit/diff if you have an idea for this working, and I'll happily consider it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah silly me forgot it's the same Self, thought it was generated. You would need to generate another module current_contract, which is a bit longer than Self. I'll think about it more because this is a pain point.

Copy link
Member

@ChaoticTempest ChaoticTempest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should attaching value to a transaction be with with_amount, with_deposit, or some other function name?

How about ext_*? It's not the most readable, but at least I think we can differentiate which methods are coming from where. And the usage of ext is less probable than with when it comes to name collisions. We could also combine the two as ext_with_*? WDYT?

Should the Ext struct be clonable? Might simplify some patterns where a common configuration is used, but also might open the API up to potential misuse

Tried playing around with it and did something along those lines:

let ext = Self::ext(env::current_account_id());
   
ext.a()
   .and(ext.b())
   .and(ext.c())
   .then(ext.handle_callbacks());

internally, wouldn't be able to control whether account_id gets cloned or not:

diff --git a/near-sdk-macros/src/core_impl/code_generator/ext.rs b/near-sdk-macros/src/core_impl/code_generator/ext.rs
index fbef6d54..960f520b 100644
--- a/near-sdk-macros/src/core_impl/code_generator/ext.rs
+++ b/near-sdk-macros/src/core_impl/code_generator/ext.rs
@@ -93,9 +93,9 @@ fn generate_ext_function(attr_signature_info: &AttrSigInfo) -> TokenStream2 {
     let Signature { generics, .. } = original_sig;
     quote! {
         #non_bindgen_attrs
-        pub fn #ident#generics(self, #pat_type_list) -> near_sdk::Promise {
+        pub fn #ident#generics(&self, #pat_type_list) -> near_sdk::Promise {
             #serialize
-            near_sdk::Promise::new(self.account_id)
+            near_sdk::Promise::new(self.account_id.clone())
             .function_call_weight(
                 #ident_str.to_string(),
                 args,

but maybe having ext.clone() would be better since we add a little more specificity but don't have to internally clone account_id:

let ext = Self::ext(env::current_account_id());
   
ext.clone().a()
   .and(ext.clone().b())
   .and(ext.c())
   .then(ext.handle_callbacks());

Not sure what the angle of attack for the abuse would be though. It seems like just a builder and cloning a builder shouldn't be bad otherwise the builder is doing too much

near-sdk-macros/src/core_impl/code_generator/ext.rs Outdated Show resolved Hide resolved
}

impl #name {
pub fn with_amount(mut self, amount: u128) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm not sure if you forgot to push your changes, but didn't we agree to rename this to with_deposit_amount?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, I was waiting for rebuilding contracts on my laptop and didn't circle back to commit and push, oops. Pushing now

Copy link
Contributor

@itegulov itegulov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just one question: did we decide not to include a check for function name clashes? In any case we can pull this in and handle in another PR, up to you

@austinabell
Copy link
Contributor Author

LGTM, just one question: did we decide not to include a check for function name clashes? In any case we can pull this in and handle in another PR, up to you

I'll open an issue and do separately

@austinabell austinabell merged commit a903f8c into master May 12, 2022
@austinabell austinabell deleted the austin/func_call_weight branch May 12, 2022 16:58
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 this pull request may close these issues.

Implement NEP264 host function and refactor ext contract API
4 participants