protofire/solhint

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

0xCLARITY opened this issue · 6 comments

0xCLARITY commented

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 commented

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

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

@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

@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

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

new release with this feature launched

  • 🎉1