Skip to content

Use mini-alloc by default and remove panic handler #373

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

Merged
merged 11 commits into from
Oct 28, 2024

Conversation

ggonzalez94
Copy link
Collaborator

@ggonzalez94 ggonzalez94 commented Oct 21, 2024

Mi intention is not that we directly merge this PR, but that it serves 2 purposes

  1. Discuss how we want to handle global allocator and mainly what to do about the panic handler
  2. In case we proceed wether we want to merge it to the release branch(and issue a new release) or just to main

Background

While testing how the experience will be for someone that creates a new stylus project by running cargo stylus new I realized a couple things that I think hurt DX.

  1. You have to disable the default feature flags on the stylus sdk(which disables mini_alloc as the default globall allocator). Otherwise you get #global_allocator conflicts as it is defined in two places(the stylus sdk and our library).
stylus-sdk = { version = "=0.6.0", default-features = false }
  1. Since our library is #[no_std] by default it defines a custom panic_handler such that if you use it from a no_std environment you don’t have to define it yourself. The problem is that if you want to use the std library in your project(and that seems to be a reasonable assumption according to this), you cannot do it since it results in a duplication of the panic_handler function. This is also the default behavior when you create a new project with cargo stylus new.

Proposed Solution

  1. Using mini-alloc by default via the sdk seems like the best approach as it was one of the recommendations from the audit and is what is used when creating a new project via cargo stylus new.
  2. Here I see two options:
    • Add a compilation flag to the panic handler such that it is not included if built with std flag on. This is the approach I took in this PR, but it has 2 downsides:
      1. If you just start a project from scratch using cargo stylus new it is harder to realize and you might spend some time trying to figure it out.
      2. We are now encouraging people to use the std flag on our library, which might enable other behaviors we haven’t considered? Since we’ve been mainly using it for testing
    • Remove the panic handler entirely from the library, allowing projects building with std to have it included by default and projects explicitly opting in to no_std and having zero dependencies on the std library would have to define it themselves.
      This approach is more reasonable IMO, since it makes it easier for newcomers, but it requires some extra effort if you are experienced enough to define your contracts with no_std .
      The problem today is that we set the library type to cdylib, which requires the crate to define a panic_handler when using no_std (more info here). If we find a way to do this, maybe this is the best approach, maybe it is possible using unstable features.

Edit

  • We were able to remove the panic handler entirely by not targeting cdylib(thanks @qalisander )! Now the user of the library can just use stdor define their own panic handler.
  • Test on a new stylus project
  • Update the Readme
  • Update Changelog

@qalisander
Copy link
Member

qalisander commented Oct 21, 2024

Seems this is similar to pr. We could discuss it there.
Do we want to merge it with 0.1.0 or main?

@bidzyyys
Copy link
Collaborator

Seems this is similar to pr. We could discuss it there. Do we want to merge it with 0.1.0 or main?

In terms of DevEx I think that we should prioritise #[no_std] environment because of e.g. reducing binary size (we know it's a big issue). I believe that once someone wants to try out Stylus, he starts with e.g. Counter example and only modifies it (without our library). BUT may want to easily add our unit tests (motsu).

I believe that our library will be used by more "mature" Stylus developers that understand the SDK and need to build more advanced projects. For this I think having well documented steps for including our library to the project is enough.

For the merge I see 2 options:

  1. Releasing quickly v0.1.1 of our library with the fix
  2. Merging to main and releasing with v0.2.0-alpha.1

@qalisander
Copy link
Member

qalisander commented Oct 21, 2024

Just updated dependencies for mini alloc. We don't need to use 0.4.2 altogether with 0.6.0 referenced by stylus-sdk.
I think we can merge this pr. And publish a patch with this fix of nightly build.

@ggonzalez94 ggonzalez94 changed the title Use mini-alloc by default and hide panic handler when usingstd Use mini-alloc by default and hide panic handler when using std Oct 21, 2024
@ggonzalez94 ggonzalez94 mentioned this pull request Oct 24, 2024
Copy link
Member

@qalisander qalisander left a comment

Choose a reason for hiding this comment

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

I've checked these changes as a patch for openzeppelin-stylus at stylus-debug-issue repo. Seems it works currently (when you call cargo clean between changes).

I suspect that probably issue was with 0.4.2 version of mini-alloc.

Copy link
Collaborator

@bidzyyys bidzyyys left a comment

Choose a reason for hiding this comment

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

Should be merged after #375 and use nigthly-2024-09-05

@ggonzalez94 ggonzalez94 changed the title Use mini-alloc by default and hide panic handler when using std Use mini-alloc by default and remove panic handler Oct 24, 2024
ggonzalez94 and others added 4 commits October 24, 2024 14:34
Resolves #292 

Sets the general structure for a Changelog that we can follow starting
with the changes for `v0.2.0`.
Structure and how to maintain it based on:
https://keepachangelog.com/en/1.1.0/

DO NOT MERGE UNTIL WE RELEASE `0.1.0` SINCE WE ARE SPECIFYING THE DATE
OF THAT RELEASE

---------

Co-authored-by: Daniel Bigos <[email protected]>
@bidzyyys bidzyyys self-requested a review October 25, 2024 10:15
Copy link
Collaborator

@bidzyyys bidzyyys left a comment

Choose a reason for hiding this comment

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

Looks fine.

…ocators

# Conflicts:
#	.github/workflows/check-wasm.yml
#	.github/workflows/e2e-tests.yml
#	.github/workflows/gas-bench.yml
@bidzyyys bidzyyys merged commit dad1921 into v0.1.0 Oct 28, 2024
20 checks passed
@bidzyyys bidzyyys deleted the improve-default-allocators branch October 28, 2024 16:28
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.

3 participants