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

Improve documentation of error codes #418

Closed
wants to merge 6 commits into from
Closed

Conversation

moisses89
Copy link
Member

Closes #386

  • Improve documentation of error codes

@coveralls
Copy link

coveralls commented Jul 5, 2022

Pull Request Test Coverage Report for Build 2703406391

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 7 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-1.6%) to 97.339%

Files with Coverage Reduction New Missed Lines %
contracts/GnosisSafeL2.sol 7 0%
Totals Coverage Status
Change from base Build 2337593609: -1.6%
Covered Lines: 294
Relevant Lines: 301

💛 - Coveralls

docs/error_codes.md Outdated Show resolved Hide resolved
docs/error_codes.md Outdated Show resolved Hide resolved
docs/error_codes.md Outdated Show resolved Hide resolved
docs/error_codes.md Outdated Show resolved Hide resolved
docs/error_codes.md Outdated Show resolved Hide resolved
docs/error_codes.md Outdated Show resolved Hide resolved
docs/error_codes.md Outdated Show resolved Hide resolved
docs/error_codes.md Outdated Show resolved Hide resolved
docs/error_codes.md Outdated Show resolved Hide resolved
docs/error_codes.md Outdated Show resolved Hide resolved
docs/error_codes.md Outdated Show resolved Hide resolved
docs/error_codes.md Outdated Show resolved Hide resolved
docs/error_codes.md Outdated Show resolved Hide resolved
docs/error_codes.md Outdated Show resolved Hide resolved
- `GS026`: `Invalid owner provided`
- **GS020: Signatures data too short**
There are less signatures than the owners threshold.
Provide as many as signatures as the owners threshold.
Copy link
Member

Choose a reason for hiding this comment

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

This is less about the signatures provide, more about the length of the bytes data. For this it probably makes sense to further explain how the signature data is created.

Copy link
Member Author

@moisses89 moisses89 Jul 18, 2022

Choose a reason for hiding this comment

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

Maybe instead add an example, add a doc link that contains examples:

The length of required signatures is less than 65*threeshold.
Each signature has a constant length of 65 bytes `{32-bytes r}{32-bytes s}{1-byte v}`. If more data is necessary it can be appended to the end. 
More information about signatures:   
https://docs.gnosis-safe.io/contracts/signatures

What do you think?

Wrong contract `v=0` signature because `startingPosition + contractSignatureLen` is out of bounds.

- **GS024: Invalid contract signature provided**
The EIP1271 signature provided is wrong.
Copy link
Member

Choose a reason for hiding this comment

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

WE should link the EIP here. Also please use EIP-1271 (which the -) ;)

- **GS024: Invalid contract signature provided**
The EIP1271 signature provided is wrong.
If you don't want to use this type of signature review the `v` value (0 for a contract signature).
The `hash` to generate the `signature` must be calculated by the account address provided in `r` value.
Copy link
Member

Choose a reason for hiding this comment

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

Not sure how helpful this comment is without further context.

Copy link
Member Author

Choose a reason for hiding this comment

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

I included that because I had some issues with this when I was working on tests for EIP-1271. To me in the past would be helpful :)

The owner provided on 'r' has not pre-approved the safeTxHash.
To pre-approve the safeTxHash call `approveHash` with the safeTxHash calculated by the owner.

- **GS026: Invalid owner provided**
Copy link
Member

Choose a reason for hiding this comment

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

We more info here. This error can happen also if the nonce has changed and therefore the Safe transaction hash is different than expected.

Review that a correct owner is being used.

- **GS031: Method can only be called from this contract**
This method is only meant to be called from the contract itself.
Copy link
Member

Choose a reason for hiding this comment

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

I would leave out descriptions for error messages that are self explanatory

@moisses89 moisses89 requested a review from rmeissner July 21, 2022 12:11
@rmeissner rmeissner closed this Aug 28, 2023
@rmeissner rmeissner deleted the error_codes_enhanced branch August 28, 2023 12:56
@github-actions github-actions bot locked and limited conversation to collaborators Aug 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve documentation of error codes
4 participants