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

Exclude abi.encodeX() calls from func-named-parameters #584

Closed
0xCLARITY opened this issue May 23, 2024 · 6 comments
Closed

Exclude abi.encodeX() calls from func-named-parameters #584

0xCLARITY opened this issue May 23, 2024 · 6 comments

Comments

@0xCLARITY
Copy link
Contributor

Solhint regularly complains about the func-named-parameters rule when all I'm doing is throwing a bunch of data into abi.encode() or abi.encodePacked().

Rather than annotating every single abi.encodeX() call with a //solhint-disable - I think it makes more sense to exclude abi.encodeX() calls from this rule at the rule implementation.

Implementation here: #583

@dbale-altoros
Copy link
Collaborator

hello @0xCLARITY thanks a lot for posting and for making a PR
Can you provide an example of what is it failing for you ?
If you are passing more than 3 unnamed parameters to the function... it should complain...
If that is the case, why do you think we need to make an exception with that function call ?
Thanks!

@0xCLARITY
Copy link
Contributor Author

So, it's not "failing" for me - I just think that all abi.encode() and abi.encodePacked() calls should be excluded from the rule.

Here's an example from some code I'm writing:

// Encoding for an EIP712 digest
abi.encode(CHANGE_USERNAME_TYPEHASH, id, username, _useNonce(custody), deadline))

abi.encode() is a function call, and so it triggers the func-named-parameters rule, because I'm passing 5 parameters.

However... it seems nonsensical to attempt to pass named parameters to an abi.encode() call. The only thing that matters in an abi.encode call is the order of the parameters. As far as I know, abi.encode() doesn't even have named parameters that you could use.

Having to disable this rule with a line comment above every abi.encode call doesn't seem useful - and I think proper behavior would be to exclude abi function calls from this rule.

(let me know if that explanation / reasoning makes sense!)

@dbale-altoros
Copy link
Collaborator

@0xCLARITY I understand what you mean now.
Thanks a lot for clarification
So in order to make it useful
We should check this at least
https://docs.soliditylang.org/en/v0.8.26/cheatsheet.html#abi-encoding-and-decoding-functions

And see also if there are others cases similar to this one to avoid this behavior
I cannot check it now... if you have time, it will be much appreciated

@0xCLARITY
Copy link
Contributor Author

0xCLARITY commented May 28, 2024

@dbale-altoros

I read through the ABI functions and checked their signatures. They are:

abi.decode(bytes memory encodedData, (...)) returns (...);
abi.encode(...) returns (bytes memory); 
abi.encodePacked(...) returns (bytes memory);
abi.encodeWithSelector(bytes4 selector, ...) returns (bytes memory);
abi.encodeCall(function functionPointer, (...)) returns (bytes memory);
abi.encodeWithSignature(string memory signature, ...) returns (bytes memory);

While there are occasional named parameters (like abi.encodeWithSelector(selector, ...)) - every function has n positional arguments that I think make them incompatible with the func-named-parameters rule.

I also checked the rest of the Solidity built-in functions to see if any of them also had n positional arguments - but it appears that only the abi.* functions do: https://solang.readthedocs.io/en/latest/language/builtins.html

I think all that is needed is excluding abi.* function calls from the rule (as currently implemented in #583 )

@dbale-altoros
Copy link
Collaborator

@0xCLARITY great one
I will review this by the end of the week or monday...
Will include this one in next version with a shootout for you
Please, review the PR because it is failing on the CI
Whenever passes all the test we can move forward with review!!

Your help is much appreciated!

@dbale-altoros
Copy link
Collaborator

new release with this feature launched

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

No branches or pull requests

2 participants