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(debug): conditionally implement debug for test errors #166

Closed
wants to merge 1 commit into from

Conversation

kaustubh1106
Copy link

Currently, all error types in the project have a default debug implementation necessary only for testing purposes. This unnecessary code in release builds increases the WASM binary size. This commit optimizes the code by implementing
#[derive(Debug)]
only for tests.

Resolves #165

PR Checklist

  • Tests
  • Documentation

Currently, all error types in the project have a default debug implementation
necessary only for testing purposes. This unnecessary code in release builds
increases the WASM binary size. This commit optimizes the code by implementing
#[derive(Debug)] only for tests.
Copy link

netlify bot commented Jun 28, 2024

Deploy Preview for contracts-stylus canceled.

Name Link
🔨 Latest commit 89c3976
🔍 Latest deploy log https://app.netlify.com/sites/contracts-stylus/deploys/667ee1eff0090400087fc4ea

Copy link

codecov bot commented Jun 28, 2024

Codecov Report

Attention: Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.

Project coverage is 90.4%. Comparing base (02d26d4) to head (89c3976).

Additional details and impacted files
Files Coverage Δ
contracts/src/access/ownable.rs 91.1% <0.0%> (ø)
contracts/src/token/erc20/extensions/capped.rs 54.5% <0.0%> (ø)
contracts/src/token/erc20/mod.rs 95.7% <0.0%> (ø)
contracts/src/token/erc721/mod.rs 96.0% <0.0%> (ø)
contracts/src/access/control.rs 90.1% <0.0%> (ø)
...ontracts/src/token/erc721/extensions/enumerable.rs 87.6% <0.0%> (ø)

@alexfertel
Copy link
Contributor

Hey @kaustubh1106, this looks perfect! Thanks for the contribution.

I see that the binary sizes remain the same after these changes, so I need to do some sanity checks to confirm my assumptions.

It may be possible that I was wrong and we are actually not including Debug implementations in the compiled binaries, which is surprising to me.

I'll report back after I get some time, sorry about this.

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.

Thanks @kaustubh1106 for your contribution. The changes you proposed looks okay, looking forward to @alexfertel research.

@alexfertel
Copy link
Contributor

Hey @kaustubh1106 , I'm really sorry, it turns out that our Debug implementations do not get included in the compiled binary. The Rust compiler does an impressive job.

You can check yourself by doing this:

  • Install twiggy with cargo install twiggy.
  • Compile the contracts with cargo build --release --target wasm32-unknown-unknown.
  • Run twiggy dominators target/wasm32-unknown-unknown/debug/erc20_example.wasm | grep Debug

For the erc20-example you would see something like this:

           3539 ┊      0.11% ┊       ⤷ <&T as core::fmt::Debug>::fmt::h48d2e8f1671d3646
           3440 ┊      0.11% ┊           ⤷ <alloy_primitives::bits::fixed::FixedBytes<_> as core::fmt::Debug>::fmt::h643dbbae3f91428f
           1225 ┊      0.04% ┊       ⤷ <alloy_sol_types::errors::Error as core::fmt::Debug>::fmt::h4709dfb49ff0aa8a
            954 ┊      0.03% ┊       ⤷ <str as core::fmt::Debug>::fmt::hd1ff027f6ce692db
            735 ┊      0.02% ┊       ⤷ <&T as core::fmt::Debug>::fmt::ha1cf56046f00303b
            636 ┊      0.02% ┊           ⤷ core::fmt::num::<impl core::fmt::Debug for u64>::fmt::hf4fb993297e90c08
            628 ┊      0.02% ┊       ⤷ core::fmt::num::<impl core::fmt::Debug for u8>::fmt::hae94439de40a9616
            566 ┊      0.02% ┊       ⤷ <&T as core::fmt::Debug>::fmt::h0ec4b1636d3511d3
            467 ┊      0.01% ┊           ⤷ <alloc::string::String as core::fmt::Debug>::fmt::h2d2026f2a243ea6d
            499 ┊      0.02% ┊       ⤷ <&T as core::fmt::Debug>::fmt::h3f2ce46b608258da
            400 ┊      0.01% ┊           ⤷ <const_hex::error::FromHexError as core::fmt::Debug>::fmt::hc78a5164bfb10119
            481 ┊      0.02% ┊       ⤷ <ruint::from::FromUintError<T> as core::fmt::Debug>::fmt::hff2a05b0c4a9a7d6
            478 ┊      0.01% ┊       ⤷ core::fmt::builders::DebugStruct::field::h04cadba0a8386f18
            460 ┊      0.01% ┊       ⤷ <alloc::borrow::Cow<B> as core::fmt::Debug>::fmt::h23fc438a588d8f87
            183 ┊      0.01% ┊           ⤷ <alloc::string::String as core::fmt::Debug>::fmt::h1531556723a98573
            450 ┊      0.01% ┊       ⤷ <core::ops::range::Range<Idx> as core::fmt::Debug>::fmt::ha51f9fe0f83a6904
            378 ┊      0.01% ┊           ⤷ core::fmt::num::<impl core::fmt::Debug for u32>::fmt::h6dbb21529d389504
            369 ┊      0.01% ┊       ⤷ core::fmt::builders::DebugTuple::field::he64b93765707a70b
            356 ┊      0.01% ┊       ⤷ <char as core::fmt::Debug>::fmt::h5885283e9b0736c5
            343 ┊      0.01% ┊       ⤷ <&T as core::fmt::Debug>::fmt::h33952ddb8df6f1c5
            244 ┊      0.01% ┊           ⤷ core::fmt::num::<impl core::fmt::Debug for usize>::fmt::hf72881040d09a72e
            339 ┊      0.01% ┊       ⤷ <&T as core::fmt::Debug>::fmt::h25d0877726be44c0
            240 ┊      0.01% ┊           ⤷ <core::option::Option<T> as core::fmt::Debug>::fmt::he1b7e235e9827490
            244 ┊      0.01% ┊       ⤷ core::fmt::num::<impl core::fmt::Debug for usize>::fmt::h4fe170eceac9ef5f
            205 ┊      0.01% ┊       ⤷ <&T as core::fmt::Debug>::fmt::h17dea41bb25002e4
            106 ┊      0.00% ┊           ⤷ <() as core::fmt::Debug>::fmt::h5bec3b654cbc4fb3
            141 ┊      0.00% ┊       ⤷ <core::array::TryFromSliceError as core::fmt::Debug>::fmt::h03acfd13ac142099
            141 ┊      0.00% ┊       ⤷ <core::array::TryFromSliceError as core::fmt::Debug>::fmt::hb9d89bee2938a4d7
            108 ┊      0.00% ┊       ⤷ <&T as core::fmt::Debug>::fmt::h23fd91eda372c7d7
             99 ┊      0.00% ┊       ⤷ <&T as core::fmt::Debug>::fmt::h04e5e56a60596699
             99 ┊      0.00% ┊       ⤷ <&T as core::fmt::Debug>::fmt::h1269f30c2d96ced6
             26 ┊      0.00% ┊       ⤷ <&T as core::fmt::Debug>::fmt::h7a9a11a22d9e302c

which doesn't include our Debug implementations, and even if it did, the impact is completely dominated by other stuff. cc: @bidzyyys @qalisander

I will close this PR and its corresponding issue since this work is no longer justified. I hope this doesn't deter you from contributing to our library in the future. Thank you very much.

@alexfertel alexfertel closed this Jul 2, 2024
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.

[Feature]: Have debug implementation conditional for erros in tests
3 participants