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

Introduce entrance crate #1223

Merged
merged 62 commits into from
Sep 15, 2022
Merged

Introduce entrance crate #1223

merged 62 commits into from
Sep 15, 2022

Conversation

ascjones
Copy link
Collaborator

@ascjones ascjones commented Apr 22, 2022

Closes #825.

⚠️ Breaking Change ⚠️

This PR:

  • Instead of introducing a new top level ink crate, simply renames the ink_lang crate to ink.
  • Reexports sub-crates from the ink crate.
  • Changes all macro generated code to use reexported crates e.g. ::ink::env instead of ::ink_env.
    • Note that for now the derives in ink_storage_derive will produce code which assumes the ink crate is imported e.g. ::ink::storage, so will not work if used as a standalone crate (see below)

Problems

So why didn't we implement this already?
The problem starts with procedural macros which are pervasive to the idea of ink!.
There currently exists no sane way to properly deal with build system related information from within a proc. macro.
For our use case it will be important to support both: using ink! crates as it happens nowadays as well as the proposed new solution via the centralized and unifying ink or ink_api crate layer.

The problem is that within a proc. macro it is hard (or impossible) to properly extract the information how exactly a project has been or will be built and therefore the proc. macro is kind of unable to properly align its generated code with respect to this. So depending on if the user actually uses this new ink_api crate or not the proc. macro is required to generate code a little differently and this is not possible without introducing impure hacks to take measures into the proc. macro itself.

In this case, specifically this is a problem with the storage derive macros. Where previously the derives, reexported via the ink_storage crate, could safely assume the presence of that crate e.g. ::ink_storage::traits::Storable. Now it generates ::ink::storage::traits::Storable instead, assuming the usage of the ink crate rather than the standalone ink_storage crate.

To overcome this in an opinionated way (for now), we will assume that a contract is uses the ink entrance crate, instead of individual crates. This means that the ink_storage macros will no longer work when using as a standalone crate.

I believe that this can be solved, but I will do that as a follow up so it easier to review and discuss. See #1400

NOTE: gitlab-examples-contract-build CI will fail because use-ink/cargo-contract#728 is required for cargo-contract to work with the new entrance crate I've temporarily updated the CI to use that PR branch to check whether all the examples still compile.

@cmichi cmichi mentioned this pull request Jul 28, 2022
@ascjones ascjones marked this pull request as ready for review September 15, 2022 11:01
@ascjones ascjones requested review from a team, cmichi and HCastano as code owners September 15, 2022 11:01
@cmichi
Copy link
Collaborator

cmichi commented Sep 15, 2022

Could you add the changes to the unreleased section of the CHANGELOG.md as well? Ideally with a note on how to migrate.

@athei
Copy link
Contributor

athei commented Sep 15, 2022

This is maybe shaving the yak but we should also re-export crates that are simply necessary for a contract to import. Otherwise it is just a chore to keep the versions in-sync. Talking about scale and scale-info.

@codecov-commenter
Copy link

codecov-commenter commented Sep 15, 2022

Codecov Report

Merging #1223 (f523263) into master (c9c81c7) will decrease coverage by 0.19%.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##           master    #1223      +/-   ##
==========================================
- Coverage   71.87%   71.68%   -0.20%     
==========================================
  Files         186      187       +1     
  Lines        5827     5844      +17     
==========================================
+ Hits         4188     4189       +1     
- Misses       1639     1655      +16     
Impacted Files Coverage Δ
crates/env/src/call/create_builder.rs 0.00% <ø> (ø)
crates/env/src/chain_extension.rs 0.00% <ø> (ø)
crates/ink/codegen/src/enforced_error.rs 100.00% <ø> (ø)
...odegen/src/generator/as_dependency/call_builder.rs 100.00% <ø> (ø)
...odegen/src/generator/as_dependency/contract_ref.rs 100.00% <ø> (ø)
...tes/ink/codegen/src/generator/as_dependency/mod.rs 100.00% <ø> (ø)
crates/ink/codegen/src/generator/blake2b.rs 100.00% <ø> (ø)
...rates/ink/codegen/src/generator/chain_extension.rs 95.74% <ø> (ø)
crates/ink/codegen/src/generator/contract.rs 100.00% <ø> (ø)
crates/ink/codegen/src/generator/dispatch.rs 94.81% <ø> (ø)
... and 167 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@cmichi cmichi left a comment

Choose a reason for hiding this comment

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

This is so nice! 🚀

Copy link
Contributor

@athei athei left a comment

Choose a reason for hiding this comment

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

I think we should renameink_lang_macro to ink_macro.

@ascjones
Copy link
Collaborator Author

ascjones commented Sep 15, 2022

This is maybe shaving the yak but we should also re-export crates that are simply necessary for a contract to import. Otherwise it is just a chore to keep the versions in-sync. Talking about scale and scale-info.

It would be nice and that was also my original intention, however it does introduce a wrinkle: when using these reexported crates for Encode, Decode, TypeInfo derives, the user would be required to add the #[codec(crate = path::to::crate)] to have those derives use the reexported path.

Of course in our codegen we can apply that attribute ourselves but if the user wants to use them like #[derive(ink::scale::Encode] they will need to do that extra step.

So I think it is definitely worth exploring to see if we can make it work, as a follow up. I would also like to see just a single crate.

@athei
Copy link
Contributor

athei commented Sep 15, 2022

I didn't think about that. This is a complication. Yeah let's put more thought into that and do it as a follow up.

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.

Towards the ink_api entrance crate
4 participants