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

Named parameters in mapping types #15

Merged
merged 2 commits into from
Apr 24, 2024
Merged

Named parameters in mapping types #15

merged 2 commits into from
Apr 24, 2024

Conversation

xenoliss
Copy link
Contributor

This PR adds the usage of named parameters in mapping types in the recommendations.

README.md Outdated
YES:

```
mapping(uint256 balanceKey => mapping(address asset => uint256 amount)) public balances;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this might be a clearer example?

Suggested change
mapping(uint256 balanceKey => mapping(address asset => uint256 amount)) public balances;
mapping(address account => mapping(address asset => uint256 amount)) public balances;

@ilikesymmetry
Copy link

+1 to naming parameters.

I'm also curious how you view plurality for mapping/array storage? Like your example, I personally prefer plural names like balances versus a singular balance.

Another pattern that relates with this is mapping visibility. For example, I recall solmate ERC20 names its mapping balanceOf and makes it public to avoid writing an extraneous function to just return the mapping value. In cases where we want to have a simple view function to return mappings, I generally still prefer having the storage name follow simpler plural form, make it internal, and define the explicit getter. In general I don't like changing storage names to match getter functions and I'm curious if either of you have opinions on that too and if they need to be clarified here.

@xenoliss xenoliss merged commit 1b4af1a into main Apr 24, 2024
3 checks passed
@xenoliss xenoliss deleted the baptiste/mappings branch April 24, 2024 09:25
@xenoliss
Copy link
Contributor Author

xenoliss commented Apr 24, 2024

@ilikesymmetry I generally agree with your take and prefer using plural for collections (arrays, mappings, list etc.).

Regarding visibility and naming, there are a few places where I think it makes sense to slightly diverge from the plural rule including, as you mentioned, matching an predefined interface without rewriting all its functions.

I generally still prefer having the storage name follow simpler plural form, make it internal, and define the explicit getter. [...] In general I don't like changing storage names to match getter functions and I'm curious if either of you have opinions on that too and if they need to be clarified here.

This is also my view on it but this is maybe not as mandatory and more subject to preferences. As we haven't discussed that yet I'm leaving it for another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants