"named-parameters-mapping" should not apply to nested mappings
PaulRBerg opened this issue · 11 comments
I have recently refactored my PRBProxy project to use named mapping parameters, and then I tried to enable the new Solhint rules named-parameters-mapping. But I got this warning:
warning Value parameter in mapping permissions is not named named-parameters-mapping
This is in spite of the fact that all params are named. It's just that the nested mapping themselves are not named, but I don't want to do this, because this doesn't make sense:
mapping(address envoy => mapping(address target => mapping(bytes4 selector => bool permission))) internal permissions;
Ok, I will review this one I think the two nested mapping inside one is causing the false positive...
But regarding the other comment...
What do you mean it doesn't make sense to name the nested ones ?
This example names the nested mapping and in my opinion it does make sense
mapping(address owner => mapping(address token => uint256 balance)) public tokenBalances
Maybe I misunderstood your point ?
Thanks, Diego!
What do you mean it doesn't make sense to name the nested ones ?
Let's take a simpler example:
mapping(uint foo => mapping(uint bar => uint256 baz));The above is totally fine. What is not fine is to name the nested mapping itself, like this:
mapping(uint foo => mapping(uint bar => uint256 baz) nestedMapping);The code above compiles but it doesn't make sense to define the nested mapping.
Also: I think the second snippet that Paul shared shouldn't be enforced, but it shouldn't be forbidden either. I don't think anyone will add names there, but forbidding it doesn't seem necessary.
Yup, agree; not enforcing but not forbidding either sounds like the best approach to take here.
Really ? The point of this addition it is just giving information about the enter key to the mapping and what the mapping contains in each "cell" on top of the mapping name...
This example that was mention (@PaulRBerg)
mapping(uint foo => mapping(uint bar => uint256 baz));
Is missing the mapping name, so what do you mean by 'it is totally fine' ?...
So, as I understand, you agree that this information only makes sense in the MAIN mapping, but it is useless in the nested ones ?
mapping(address owner => mapping(address token => uint256 balance)) public tokenBalances
This one shows info about the main enter key, the second enter key and the content... Don't see anything that doesn't make sense here... At most it is a bit 'over explained' ?
But it's ok. You guys are here long before me and your approach is simpler in terms of coding... I can fix it that way...
So if the rule is ON:
Will check only the naming of the KEY of the MAIN mapping and give an error if it is not present
And left alone the other named parameters on the mapping (doesn't matter if they exist or not)... no report of them...
Is that OK ?
@fvictorio
Shoot, I made an error in the OP. I meant to say that the name of the parent mapping SHOULD be enforced, but not the names of the nested mappings. I forgot to name the parent mapping in my example.
you agree that this information only makes sense in the MAIN mapping, but it is useless in the nested ones ?
Yes.
Will check only the naming of the KEY of the MAIN mapping and give an error if it is not present
Yes.
@PaulRBerg what about a simple mapping ?
Following the logic of your example... this is OK:
mapping(address token => uint256) public tokenBalances;
and this is also ok
mapping(address token => uint256 balance) public tokenBalances;
Naming of the uint256 (balance) should not be enforced in a simple mapping, right ?
Or the no-enforcement applies only to nested mappings ?
I was mostly thinking about nested mappings; I think the naming should still be enforced for values because they may not be inferable from the name of the parent mapping.
@PaulRBerg
Ok , so to be clear. The rule will work as follow:
For regular mapping, this is the only way to write them and both variable naming should be enforced:
mapping(address token => uint256 balance) public tokenBalances;
For nested mapping
- The MAIN KEY variable (the entrance of the main mapping) should be named and this is enforced...
- Every other variable can be named or not... this is not enforced
Yes.
fixed on #421