[DOCS] Add clarifications to mapping type #7100

ChrisChinchilla · 2019-07-15T10:57:01Z

Description

Closes #6303 by adding some description around the code example, and adding a link to the type when mentioned in other places.

Checklist

Code compiles correctly
All tests are passing
New tests have been created which fail without the change (if possible)
README / documentation was extended, if necessary
Changelog entry (if change is visible to the user)
Used meaningful commit messages

docs/types/mapping-types.rst

chriseth · 2019-07-15T14:57:15Z

docs/types/mapping-types.rst

+
+In the example below, the ``MappingExample`` contract defines a public ``balances``
+mapping, with the key type an ``address``, and a value type a ``uint``, mapping
+an integer value to an Ethereum address. As ``uint`` is a value type, the getter


Mapping an address to an unsigned integer.

ChrisChinchilla · 2019-07-24T10:22:09Z

@chriseth I understand your last comment, and I understand the principles, but as this was the content that was already there, I'm struggling slightly to explain it better.

The pseudo syntax refers to the earlier paragraph, and I am hoping that the example (and explanation) help explain it…

chriseth · 2019-08-05T09:00:09Z

docs/types/mapping-types.rst

@@ -4,7 +4,7 @@
 Mapping Types
 =============

-You declare mapping types with the syntax ``mapping(_KeyType => _ValueType)``.
+You declare mapping types with the syntax ``mapping(_KeyType => _ValueType) _VariableModifiers _VariableName``.


This is how you declare a variable of mapping type, not how you declare a mapping type.

Maybe declare is also the wrong word, use may be better.

chriseth · 2019-08-05T09:01:14Z

docs/types/mapping-types.rst

-:ref:`getter <visibility-and-getters>` for you. The ``_KeyType`` becomes a
-parameter for the getter. If ``_ValueType`` is a value type or a struct,
-the getter returns ``_ValueType``.
+:ref:`getter <visibility-and-getters>` for you. The ``_KeyType`` is the parameter for the getter.


Please keep "becomes a", because it may not be the only parameter.

ChrisChinchilla · 2019-08-05T10:51:03Z

Changes made

chriseth · 2019-08-05T14:57:16Z

docs/types/mapping-types.rst

@@ -4,7 +4,8 @@
 Mapping Types
 =============

-You declare mapping types with the syntax ``mapping(_KeyType => _ValueType)``.
+Mapping types use the syntax ``mapping(_KeyType => _ValueType)`` and variables
+are declared as a mapping type using the syntax ``mapping (_KeyType => _ValueType) _VariableModifiers _VariableName``.


It's probably not too useful to keep this.

I still do not think it is useful to keep this.

Which bit? It was here already, and from the conversations I've had around, people are really confused by the mapping syntax, so I'd love to try and be as explicit and clear as possible. Any suggestions to keep that clarity but express it differently?

I think it would be better to give an example how to declare a specific variable instead of just adding random placeholders whose meaning is not explained.

docs/types/mapping-types.rst

ChrisChinchilla · 2019-08-26T18:02:21Z

docs/types/mapping-types.rst

+point to remember are that the mapping inside the mapping is not a return variable,
+but the variable inside it is.
+
+::


@chriseth Myself and @ekpyron had a chat about an example for a mapping with a mapping and struggled to think of many great examples. What do you think of this one? Then I'll polish the text.

looks good!

chriseth · 2019-08-28T15:44:15Z

docs/types/mapping-types.rst

@@ -52,6 +58,34 @@ each ``_KeyType``, recursively. For example with a mapping:
        }
    }

+The example below uses a mapping type inside another mapping type. The important
+point to remember are that the mapping inside the mapping is not a return variable,
+but the variable inside it is.


"value" instead of 'variable' for both?

docs/types/mapping-types.rst

chriseth · 2019-08-28T15:44:38Z

docs/types/mapping-types.rst

+        function f() public returns (bool) {
+            MappingExample m = new MappingExample();
+            m.transfer(address(this),address(this));
+            return m.validBalances(address(this),address(this));


Suggested change

return m.validBalances(address(this),address(this));

return m.validBalances(address(this), address(this));

chriseth · 2019-08-28T15:45:24Z

docs/types/mapping-types.rst

+    pragma solidity >=0.4.0 <0.7.0;
+
+    contract MappingExample {
+        mapping(address => mapping(address => bool)) public validBalances;


Doesn't erc20 use something like allowances?

It does, but I'm not sure what you mean? This isn't really an ERC20 implementation, and the data structure here is different too.

No, it's exactly the same - and I think it would be a good example: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol#L36

ChrisChinchilla · 2019-08-30T13:36:56Z

@chriseth Made changes, i think I corrected another code error too.

chriseth · 2019-09-04T17:12:38Z

Tests are failing.

chriseth · 2019-09-09T13:55:22Z

docs/types/mapping-types.rst

+each ``_KeyType``, recursively.
+
+In the example below, the ``MappingExample`` contract defines a public ``balances``
+mapping, with the key type an ``address``, and a value type a ``uint``, mapping


Isn't it mapping the address to the uint?

I think that is what I was trying to say, but I'll make it clearer.

chriseth · 2019-09-09T13:56:05Z

docs/types/mapping-types.rst

+
+    pragma solidity >=0.4.0 <0.7.0;
+
+    contract MappingExample {


Do you think a full credit / allowance example would be too long?

@chriseth Possibly, but how much do you think the full example relates to mappings and how they work? Or maybe a reduced example of what you suggest that highlights those aspects? I assume based on the Zeppelin link from above?

I think you can use the following functions and also slim them down a little:

approve

transferFrom

allowances

ekpyron · 2019-09-10T13:24:08Z

docs/types/mapping-types.rst

@@ -52,6 +58,34 @@ each ``_KeyType``, recursively. For example with a mapping:
        }
    }

+The example below uses a mapping type inside another mapping type. The important
+point to remember are that the mapping inside the mapping is not the return value,


That should be The important point to remember is not are... but also: "the return value" of what? I'm not sure I understand the purpose of this sentence :-).

Corrected that. The point is to clarify that the returned value isn't the mapping inside the mapping, but the value inside the nested mapping. That's what I understood from you anyway?

When you're talking about "the returned value", you have to say what you are talking about that returns something - the index access of the outer mapping? The index access of the outer mapping in fact does return "the mapping inside the mapping" - only the second index access yields the value contained in the inner mapping...

contract C { mapping(address => mapping(address => bool)) public allowances; function f(address X, address Y) public view { mapping(address => bool) storage allowancesForX = allowances[X]; // first index access is a mapping bool allowanceForXandY = allowances[X][Y]; // two index accesses give the contained value bool allowanceForXandY2 = allowancesForX[Y]; // which is the same as accessing the contained mapping at Y assert(allowanceForXandY == allowanceForXandY2); } }

ChrisChinchilla · 2019-09-13T17:08:39Z

OK @chriseth done!

I had to cut a fair bit out, so I hope nothing got lost in the process. @ekpyron this also cut most of the copy that we were discussing, possibly it even needs more explanation now!

docs/types/mapping-types.rst

chriseth · 2019-09-17T09:42:02Z

docs/types/mapping-types.rst

+
+        function transferFrom(address sender, address recipient, uint256 amount) public returns (bool) {
+            _transfer(sender, recipient, amount);
+            _approve(sender, msg.sender, amount);


Why do you call approve here? It should rather check that the allowance is large enough.

@chriseth This came from the Zeppelin code…

But it does something else now.

erak

Needs to rebased since the failing test was fixed with #7419.

ChrisChinchilla · 2019-10-07T03:43:16Z

@erak Rebased, squashed

chriseth · 2019-10-15T14:31:43Z

docs/types/mapping-types.rst

+In the example below, the ``MappingExample`` contract defines a public ``balances``
+mapping, with the key type an ``address``, and a value type a ``uint``, mapping
+an Ethereum address to an unsigned integer value. As ``uint`` is a value type, the getter
+returns a value that matches the type, which you can see in the ``MappingUser``


"of that type" (how does a value match a type?)

chriseth · 2019-10-15T14:34:01Z

docs/types/mapping-types.rst

+            emit Transfer(sender, recipient, amount);
+        }
+
+        function _approve(address owner, address spender, uint256 amount) internal {


I think it is better to combine this function with approve and handle the approval reduction in transferFrom manually.

I think this is what you're asking for…

Updates from review Changes from review Nested example Update docs/types/mapping-types.rst Co-Authored-By: chriseth <[email protected]> Changes from review Bring example inline with ERC20 Clarify what maps where Use OZ contract example Update docs/types/mapping-types.rst Co-Authored-By: chriseth <[email protected]> update code example

chriseth · 2019-11-06T20:32:39Z

Rebased.

chriseth

Still not sure about some details, but better to merge this.

chriseth reviewed Jul 15, 2019

View reviewed changes

docs/types/mapping-types.rst Show resolved Hide resolved

chriseth reviewed Jul 15, 2019

View reviewed changes

ChrisChinchilla force-pushed the docs-review-mappings branch from 3d1bc27 to 9d73c76 Compare July 24, 2019 10:17

ChrisChinchilla force-pushed the docs-review-mappings branch from 9d73c76 to 87dbebf Compare July 31, 2019 10:09

chriseth reviewed Aug 5, 2019

View reviewed changes

ChrisChinchilla force-pushed the docs-review-mappings branch from 87dbebf to 2871985 Compare August 5, 2019 10:50

chriseth reviewed Aug 5, 2019

View reviewed changes

docs/types/mapping-types.rst Show resolved Hide resolved

ChrisChinchilla force-pushed the docs-review-mappings branch from 2871985 to 076f0dc Compare August 26, 2019 05:33

ChrisChinchilla commented Aug 26, 2019

View reviewed changes

chriseth reviewed Aug 28, 2019

View reviewed changes

docs/types/mapping-types.rst Outdated Show resolved Hide resolved

chriseth reviewed Aug 28, 2019

View reviewed changes

ChrisChinchilla force-pushed the docs-review-mappings branch from 8f2d068 to 6ea5578 Compare September 9, 2019 11:24

chriseth reviewed Sep 9, 2019

View reviewed changes

ChrisChinchilla force-pushed the docs-review-mappings branch from 967a3e1 to 08e26b4 Compare September 9, 2019 15:40

ekpyron reviewed Sep 10, 2019

View reviewed changes

chriseth reviewed Sep 17, 2019

View reviewed changes

docs/types/mapping-types.rst Outdated Show resolved Hide resolved

chriseth reviewed Sep 17, 2019

View reviewed changes

erak requested changes Oct 4, 2019

View reviewed changes

ChrisChinchilla force-pushed the docs-review-mappings branch 2 times, most recently from a391523 to ddc9282 Compare October 7, 2019 03:42

chriseth reviewed Oct 15, 2019

View reviewed changes

ChrisChinchilla force-pushed the docs-review-mappings branch from ddc9282 to 5884d78 Compare October 18, 2019 08:16

ChrisChinchilla force-pushed the docs-review-mappings branch from 5884d78 to 87367b4 Compare November 5, 2019 16:11

ChrisChinchilla requested a review from erak November 5, 2019 16:29

chriseth previously approved these changes Nov 6, 2019

View reviewed changes

chriseth dismissed their stale review via 0c51dcc November 6, 2019 20:32

chriseth force-pushed the docs-review-mappings branch from 87367b4 to 0c51dcc Compare November 6, 2019 20:32

chriseth approved these changes Nov 6, 2019

View reviewed changes

chriseth merged commit efeee15 into develop Nov 6, 2019

chriseth deleted the docs-review-mappings branch November 6, 2019 20:36

	return m.validBalances(address(this),address(this));
	return m.validBalances(address(this), address(this));

[DOCS] Add clarifications to mapping type #7100

[DOCS] Add clarifications to mapping type #7100

Conversation

ChrisChinchilla commented Jul 15, 2019 • edited Loading

Description

Checklist

Choose a reason for hiding this comment

ChrisChinchilla commented Jul 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisChinchilla commented Aug 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisChinchilla commented Aug 30, 2019

chriseth commented Sep 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisChinchilla commented Sep 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erak left a comment

Choose a reason for hiding this comment

ChrisChinchilla commented Oct 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseth commented Nov 6, 2019

chriseth left a comment

Choose a reason for hiding this comment

ChrisChinchilla commented Jul 15, 2019 •

edited

Loading