protofire / solhint

Solhint is an open-source project to provide a linting utility for Solidity code.
https://protofire.github.io/solhint/
MIT License
1.01k stars 156 forks source link

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

Open 0xCLARITY opened 1 month ago

0xCLARITY commented 1 month ago

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: https://github.com/protofire/solhint/pull/583

dbale-altoros commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

@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 commented 1 month ago

@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!