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

Removed the need to use Self::Api #1252

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

gfusee
Copy link
Contributor

@gfusee gfusee commented Oct 25, 2023

Abstract

In the current state of the sdk, the user has the choice to either add Self::Api for every operation he does, or relying on the macros doing it on his behalf. While the first choice is clearly inconvenient, the second one is also a big deal because the Self::Api is appended in a procedural macro (multiversx_sc::contract, multiversx_sc::module or multiversx_sc::proxy) and have the following major drawbacks:

  • The IDE must be capable of expending the macro in real-time
  • Macro expansion is only possible if the developer's code is valid, this is rare in the development process.
  • In the case where it is possible, macro expansion takes time and consume resources
  • Some specific cases are hard when it comes to append generics parameters, for example custom structs
  • Using a such "API" parameter makes beginner confuse

What is this pull request about?

This pull request is not intended to be merge as is, the code is not clean and some tests don't pass. It is a draft to see what could be a potential solution to the above problematic.

I made a LOT of changes to make the VMApi parameter instantiated at compile time. The first notable change is the renaming of the known managed type and mappers to append the prefix "Base". I only did it for BigUint and SingleValueMapper which become BaseBigUint and BaseSingleValueMapper.

The next change was to modify the multiversx_sc::imports! macro, I added two things:

  • The call to a procedural macro multiversx_sc::api_imports! which declares a type CurrentApi depending of both the target architecture (wasm or not) and crate's feature (to choose between the DebugApi, StaticApi, SingleTxApi, etc...)
  • The redeclaration of the the managed types without the "Base" prefix, in a way where the CurrentApi type is used. For example type BigUint = BaseBigUint<CurrentApi> or type SingleValueMapper<T> = BaseSingleValueMapper<CurrentApi, T>. This is done in the declarative macro instead of a procedural one, so the developer can immediately access to the IDEs features (autocompletions, error handling, etc...)

Advantages

  • Developer experience greatly improved, especially for beginners
  • Code is more readable (compared to a code containing Self::Api everywhere)
  • CurrentApi is available in every place where the imports! macro is invoked. So macros like sc_print!, sc_panic!, etc... work outside a contract or module trait. Moreover, it is possible to remove every reference to Api parameters in custom structs & enums
  • No need to check for Self::Api appending in the macro -> better build performances

Drawbacks

  • Since the choice between which Api to use is done at compile time, there are some lines to add to the Cargo.toml
  • It is harder to test the mx-sdk-repo since we should run multiple tests command for each selected Api and ignore tests that use different Api from those selected
  • Unable to have multiple APIs in the binary, some workaround are possible

What's not working

Since the goal is not to necessarily merge this pull request, I only made the core functionalities to work, here are what's not working currently:

  • Multi contract system, all related tests fail
  • Some changes are needed in the wasm crate of each contract, I only did them in the adder contract for testing purpose. Other contracts will probably not compile when targeting wasm
  • the sc-meta CLI, I didn't have the time to change it to take in account features
  • the "cargo build" command, didn't have the time to make it working. However excepted for the sc-meta CLI, this command is never used. Developers use the command "cargo build --tests", or "cargo test", those commands work.
  • Some things can go wrong when running a build from a workspace, didn't figure out why. From my last try the "cargo test" don't fail building the project when ran from the workspace's root
  • The code is not clean

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.

1 participant