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

[Feature]: Investigate How to Address export-abi Warnings #439

Open
1 task done
0xNeshi opened this issue Dec 9, 2024 · 2 comments
Open
1 task done

[Feature]: Investigate How to Address export-abi Warnings #439

0xNeshi opened this issue Dec 9, 2024 · 2 comments
Labels
effort: medium Default level of effort. type: bug Something is not working as intended. uphill Some research/design is needed before this can be implemented.

Comments

@0xNeshi
Copy link
Collaborator

0xNeshi commented Dec 9, 2024

What is the feature you would like to see?

By declaring export-abi feature, we can remove those pesky warnings.
The problem is that cargo hack workflow fails when it tried to activate this feature with the trait `SolError` is not implemented for `[OUR_ERROR_NAME]` .
We should investigate why this is, so that we can declare the feature in Cargo.toml.

Alternatively, we can simply skip this feature, just as we do for std.

Original comment: #437 (comment)

Related #436

Contribution Guidelines

  • I agree to follow this project's Contribution Guidelines
@0xNeshi 0xNeshi added needs triage Needs to be assigned the appropriate labels type: chore A maintenance-related change. labels Dec 9, 2024
@0xNeshi 0xNeshi changed the title [Feature]: Investigate Why Enabling export-abi Feature Causes Issues [Feature]: Investigate How to Disable export-abi Warnings Dec 9, 2024
@0xNeshi 0xNeshi self-assigned this Dec 9, 2024
@0xNeshi
Copy link
Collaborator Author

0xNeshi commented Dec 9, 2024

Declaring the export-abi feature in all relevant _Cargo.toml_s, and running cargo doc or cargo check with this feature enabled results in the following errors (I specifically ran RUSTDOCFLAGS="--cfg docsrs" cargo +nightly doc --no-deps --all-features):

  • all our contract functions that return Self::Error as part of their result (i.e. Result<[some_type], Self::Error>) fail with the following (example for ownable_two_step.rs):
error[E0220]: associated type `Error` not found for `Self`
   --> contracts/src/access/ownable_two_step.rs:158:27
    |
158 |     ) -> Result<(), Self::Error> {
    |                                       ^^^^^ associated type `Error` not found

Mitigation: this is a very weird error that is solved using a fully-qualified path for Self, like: Result<(), <Self as IOwnable2Step>::Error>. I presume this is an issue in #[public] macro, as this error only appears when --export-abi feature is enabled.

  • The already reported "SolError is not implemented" error appears for various errors, e.g. for ownable_two_step.rs:
error[E0277]: the trait bound `ownable::Error: SolError` is not satisfied
  --> contracts/src/access/ownable_two_step.rs:48:13
   |
48 |     Ownable(OwnableError),
   |             ^^^^^^^^^^^^ the trait `SolError` is not implemented for `ownable::Error`
   |
   = help: the following other types implement trait `SolError`:
             AccessControlBadConfirmation
             AccessControlUnauthorizedAccount
             CheckpointUnorderedInsertion
             ECDSAInvalidSignature
             ECDSAInvalidSignatureS
             ERC1155InsufficientBalance
             ERC1155InvalidApprover
             ERC1155InvalidArrayLength
           and 39 others

What's worth noting is that this error only appears for the wrapped errors from other contracts. In other words, in each place where we "inherit" errors from another contract to be able to wrap and return them, SolError is not implemented (it is implemented only for those errors defined within sol! macro), and as the --export-abi feature requires all errors to implement it, we get an issue.
@bidzyyys @qalisander this is an important point! Since ABI's cannot be generated by the SDK for our contracts, this could mean that our contracts could be undeployable (EDIT: Our contracts are deployable, I checked, see edit at the end), and they just happen to pass local e2e tests!!

Did anyone try to actually deploy any of our contracts that inherit errors (e.g. Erc20Permit)?

@qalisander might this be related to a similar issue you raised with the Stylus team OffchainLabs/stylus-sdk-rs#135?


EDIT: I created a new Stylus project, set our lib as a dependency and created an Erc20Permit, as per our example. I then ran the following stylus CLI commands to see the results:

  • cargo stylus check - passes ✔️
  • cargo stylus deploy - passes 😌 ✔️
  • cargo stylus export-abi - fails with our warnings turned into errors ❌

It seems we will need to handle this issue somehow, as users of our library won't be able to use the export-abi command.
Mitigation 1: Stylus team addresses OffchainLabs/stylus-sdk-rs#135 asap.
Mitigation 2: Instead of inheriting errors from other contracts through the enum Error, we inherit them by adding them one-by-one into the inheriting contract's error enum.
So instead of:

use crate::utils::cryptography::ecdsa;

pub enum Error {
    // ...
    ECDSA(ecdsa::Error),
}

We do:

use crate::utils::cryptography::ecdsa::{
    ECDSAInvalidSignature, ECDSAInvalidSignatureS,
};

pub enum Error {
    // ...
    // add errors directly
    InvalidSignature(ECDSAInvalidSignature),
    InvalidSignatureS(ECDSAInvalidSignatureS),
}

@0xNeshi 0xNeshi added type: bug Something is not working as intended. effort: medium Default level of effort. uphill Some research/design is needed before this can be implemented. and removed needs triage Needs to be assigned the appropriate labels type: chore A maintenance-related change. labels Dec 10, 2024
@0xNeshi 0xNeshi changed the title [Feature]: Investigate How to Disable export-abi Warnings [Feature]: Investigate How to Address export-abi Warnings Dec 11, 2024
@0xNeshi
Copy link
Collaborator Author

0xNeshi commented Dec 11, 2024

To recap:

Until these two are resolved, this issue is considered blocked

@0xNeshi 0xNeshi removed their assignment Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: medium Default level of effort. type: bug Something is not working as intended. uphill Some research/design is needed before this can be implemented.
Projects
Status: Todo
Development

No branches or pull requests

1 participant