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

refactor: replace syscall interactions in wasm32 environment #417

Merged
merged 24 commits into from
Jul 7, 2021

Conversation

austinabell
Copy link
Contributor

@austinabell austinabell commented May 13, 2021

  • Removes need to go through thread local, refcell, dynamic dispatch just to call syscalls in wasm32 environments through their host functions
    • I also hid a lot of the related logic under the wasm32 config (and might add more) which might be breaking if currently misused externally

Depends on #415 (not necessarily directly, but makes a lot of this a lot cleaner)

I tested with all examples and they all work, will go on to text external things now to check for breakages

Closes #373
Closes #416

Progress on #372

Copy link
Contributor

@matklad matklad left a comment

Choose a reason for hiding this comment

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

-200 lines look nice!

@@ -12,14 +12,10 @@ use syn::{File, ItemEnum, ItemImpl, ItemStruct, ItemTrait};
#[proc_macro_attribute]
pub fn near_bindgen(_attr: TokenStream, item: TokenStream) -> TokenStream {
if let Ok(input) = syn::parse::<ItemStruct>(item.clone()) {
let sys_file = rust_file(include_str!("../res/sys.rs"));
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

@austinabell
Copy link
Contributor Author

austinabell commented May 14, 2021

Think we need to make the sys module public to allow for contracts, like ref-finance, to retain low level controls that were previously going through the BLOCKCHAIN_INTERFACE.

For now, I'll just make it public, but I'd love opinions on how this should be namespaced (within env or exposed at root level) and if this should live under the unstable feature flag

Edit: ignore the path references, because I didn't want to update to a pushed commit yet, but austinabell/ref-contracts@26ebf5b gives an idea of how the migration could go. Builds and tests pass using this branch. Does anyone have a good example of a contract that uses a lot of the sdk's internals?

Base automatically changed from austin/migrate_typedefs to master May 15, 2021 00:48
@austinabell austinabell marked this pull request as ready for review May 17, 2021 16:46
@austinabell
Copy link
Contributor Author

I put the sys interface (syscalls) under an unstable API for now. There are still some necessary interactions that were previously using the BLOCKCHAIN_INTERFACE directly in the smart contract, but ideally, this would have some safe abstraction on top of it to avoid exposing the raw syscalls to a developer, which could change in the future.

This will absolutely be a breaking change but would be very good to get in if working towards having a stable contract upgradability pattern to use this updated interface.

@austinabell austinabell requested a review from matklad May 17, 2021 16:49
@matklad
Copy link
Contributor

matklad commented May 19, 2021

Think we need to make the sys module public to allow for contracts, like ref-finance, to retain low level controls that were previously going through the BLOCKCHAIN_INTERFACE.

Oh dear, so we actually publicly exposed the whole edifice, with thread_local and such?

I think the solution of exposing the naked sys module is a good one! I'd probably keep it at the top-level near_sdk::sys and not in environment::sys, but that's a minor point. The important one is that, from the outside, only near_sdk::sys is visible, and I think we are good on that front. We might also consider publishing a near-sys crate in the future, so that it's easy to build alternative sdks, but I don't think necessary near-term (squatted the name non the less). If we publish the sys-crate, we can backwards compatible re-export it as a sys module.

The question is, what do we do with version numbers though?

  • we can publish this as a new major version, but that will add needless churn
  • we can publish this in a minor version, pulling a semver trick and saying that this wasn't a documented public API anyway,

@austinabell
Copy link
Contributor Author

Oh dear, so we actually publicly exposed the whole edifice, with thread_local and such?

I think the solution of exposing the naked sys module is a good one! I'd probably keep it at the top-level near_sdk::sys and not in environment::sys, but that's a minor point. The important one is that, from the outside, only near_sdk::sys is visible, and I think we are good on that front.

Yeah, it was exposed, so this is definitely a breaking change. I have exposed it as sys but put it under the unstable flag because ideally there would be a safe interface to do any interactions necessary. Good to go on this or do you think there should be anything changed here?

We might also consider publishing a near-sys crate in the future, so that it's easy to build alternative sdks, but I don't think necessary near-term (squatted the name non the less). If we publish the sys-crate, we can backwards compatible re-export it as a sys module.

Yeah, this is definitely a good idea, how about we keep the interface under unstable for a bit and see what exactly will be needed/can be abstracted, then move it into this other crate after some concrete use?

@matklad
Copy link
Contributor

matklad commented May 19, 2021

Yeah, this is definitely a good idea, how about we keep the interface under unstable for a bit and see what exactly will be needed/can be abstracted, then move it into this other crate after some concrete use?

👍

Yeah, this is definitely a good idea, how about we keep the interface under unstable for a bit and see what exactly will be needed/can be abstracted, then move it into this other crate after some concrete use?

I think we need to explicitly decide what we do with respect to version numbers & publishing. Thinking about this some more, the choices are:

  • make a minor release ASAP, partners who use low-level interface will be seriously annoyed, other partners wont notice
  • collect any other super major changes we want to make, and which we can't make later, and release a major ASAP. People who use low-level API will be annoyed by churn, but would love simpler sys calls. People who don't use low-level API will be slightly annoyed by a major revision which doesn't actually improve anything for them. We loose some "stability" marketing points
  • fold other API cleanups (like strongly type AccountIds and such) into this change, and release a new major much later.
  • bite the bullet and just preserve API compat. That is, leave thread_local and dyn Trait there, but implement the trait in terms of sys module

@matklad
Copy link
Contributor

matklad commented May 19, 2021

To me it seems that the least controversial option is 2, burn yet another major version number. It has the fewest unknown unknowns.

@austinabell
Copy link
Contributor Author

I think we need to explicitly decide what we do with respect to version numbers & publishing. Thinking about this some more, the choices are:

  • make a minor release ASAP, partners who use low-level interface will be seriously annoyed, other partners wont notice
  • collect any other super major changes we want to make, and which we can't make later, and release a major ASAP. People who use low-level API will be annoyed by churn, but would love simpler sys calls. People who don't use low-level API will be slightly annoyed by a major revision which doesn't actually improve anything for them. We loose some "stability" marketing points
  • fold other API cleanups (like strongly type AccountIds and such) into this change, and release a new major much later.
  • bite the bullet and just preserve API compat. That is, leave thread_local and dyn Trait there, but implement the trait in terms of sys module

Yeah maybe option 2 is the best option, and I'm having a tough time making a point for another.

I would really like to avoid keeping the BLOCKCHAIN_INTERFACE as it has literally no benefit and creates some really ugly code patterns to interact with directly.

Let's create a list of all things that should change for a major release and try to work to that ASAP (while limiting as many breaking changes), and can always decide to defer making these changes if the scoped out list is not suitable for external/internal parties. Maybe we can use #387 for coordination on this?

@austinabell austinabell mentioned this pull request May 20, 2021
10 tasks
Copy link
Contributor

@mikedotexe mikedotexe left a comment

Choose a reason for hiding this comment

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

Thanks for the discussion in our meeting about this today. 👍🏼

feat: Initialize blockchain interface by default
@austinabell austinabell merged commit 5cc2a33 into master Jul 7, 2021
@austinabell austinabell deleted the austin/extract_vmlogic branch July 7, 2021 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants