Erc721 governance documentation improvement

contracts/governance/README.adoc

@@ -22,8 +22,6 @@ Votes modules determine the source of voting power, and sometimes quorum number.
 
 * {GovernorVotes}: Extracts voting weight from an {ERC20Votes} token.
 
-* {GovernorVotesERC721}: Extracts voting weight from an {ERC721Votes} token.
-
 * {GovernorVotesComp}: Extracts voting weight from a COMP-like or {ERC20VotesComp} token.
 
 * {GovernorVotesQuorumFraction}: Combines with `GovernorVotes` to set the quorum as a fraction of the total token supply.
@@ -86,12 +84,17 @@ NOTE: Functions of the `Governor` contract do not include access control. If you
 
 {{GovernorProposalThreshold}}
 
+== Utils
+
+{{Votes}}
+
 == Timelock
 
 In a governance system, the {TimelockController} contract is in charge of introducing a delay between a proposal and its execution. It can be used with or without a {Governor}.
 
 {{TimelockController}}
 
+
 [[timelock-terminology]]
 ==== Terminology
 

contracts/governance/extensions/GovernorVotes.sol

@@ -7,7 +7,7 @@ import "../Governor.sol";
 import "../utils/IVotes.sol";
 
 /**
- * @dev Extension of {Governor} for voting weight extraction from an {ERC20Votes} or {ERC721Votes} token.
+ * @dev Extension of {Governor} for voting weight extraction from an {ERC20Votes} token, or since v4.5 an {ERC721Votes} token.
  *
  * _Available since v4.3._
  */

contracts/governance/utils/Votes.sol

@@ -4,11 +4,12 @@ pragma solidity ^0.8.0;
 import "../../utils/Context.sol";
 import "../../utils/Counters.sol";
 import "../../utils/Checkpoints.sol";
-import "./IVotes.sol";
 import "../../utils/cryptography/draft-EIP712.sol";
+import "./IVotes.sol";
 
 /**
- * @dev Voting operations.
+ * @dev This is a base abstract contract that tracks voting power for a set of accounts with a vote delegation system.
+ * It can be combined with a token contract to represent voting power as the token unit, see {ERC721Votes}.
  *
  * This extension keeps a history (checkpoints) of each account's vote power. Vote power can be delegated either
  * by calling the {delegate} function directly, or by providing a signature to be used with {delegateBySig}. Voting
@@ -19,8 +20,10 @@ import "../../utils/cryptography/draft-EIP712.sol";
  * Enabling self-delegation can easily be done by overriding the {delegates} function. Keep in mind however that this
  * will significantly increase the base gas cost of transfers.
  *
- * When using this module, the derived contract must implement {_getDelegatorVotingPower}, and can use {_moveVotingPower}
+ * When using this module, the derived contract must implement {_getDelegatorVotingPower}, and can use {_transferVotingAssets}
  * when a delegator's voting power is changed.
+ *
+ * _Available since v4.5._
  */
 abstract contract Votes is IVotes, Context, EIP712 {
     using Checkpoints for Checkpoints.History;
@@ -53,14 +56,14 @@ abstract contract Votes is IVotes, Context, EIP712 {
     }
 
     /**
-     * @dev Returns total amount of votes at given blockNumber.
+     * @dev Returns total amount of votes at given blockNumber for account.
      */
     function getPastVotes(address account, uint256 blockNumber) public view virtual override returns (uint256) {
         return _delegateCheckpoints[account].getAtBlock(blockNumber);
     }
 
     /**
-     * @dev Retrieve the `totalVotingPower` at the end of `blockNumber`. Note, this value is the sum of all balances.
+     * @dev Retrieve the votes total supply at the end of `blockNumber`. Note, this value is the sum of all balances.
      * It is but NOT the sum of all the delegated votes!
      *
      * Requirements:
@@ -75,7 +78,7 @@ abstract contract Votes is IVotes, Context, EIP712 {
     /**
      * @dev Returns total amount of votes.
      */
-    function _getTotalVotes() internal view virtual returns (uint256) {
+    function _getTotalSupply() internal view virtual returns (uint256) {
         return _totalCheckpoints.latest();
     }
 
@@ -130,24 +133,37 @@ abstract contract Votes is IVotes, Context, EIP712 {
         _moveVotingPower(oldDelegation, newDelegation, _getDelegatorVotingPower(delegator));
     }
 
+    /**
+     * @dev Transfers voting assets.
+     */
+    function _transferVotingAssets(
+        address from,
+        address to,
+        uint256 amount
+    ) internal virtual {
+        if (from == address(0)) {
+            _totalCheckpoints.push(_add, amount);
+        }
+        if (to == address(0)) {
+            _totalCheckpoints.push(_subtract, amount);
+        }
+        _moveVotingPower(delegates(from), delegates(to), amount);
+    }
+
     /**
      * @dev Moves voting power.
      */
     function _moveVotingPower(
         address from,
         address to,
         uint256 amount
-    ) internal virtual {
+    ) private {
         if (from != to && amount > 0) {
-            if (from == address(0)) {
-                _totalCheckpoints.push(_add, amount);
-            } else {
+            if (from != address(0)) {
                 (uint256 oldValue, uint256 newValue) = _delegateCheckpoints[from].push(_subtract, amount);
                 emit DelegateVotesChanged(from, oldValue, newValue);
             }
-            if (to == address(0)) {
-                _totalCheckpoints.push(_subtract, amount);
-            } else {
+            if (to != address(0)) {
                 (uint256 oldValue, uint256 newValue) = _delegateCheckpoints[to].push(_add, amount);
                 emit DelegateVotesChanged(to, oldValue, newValue);
             }
@@ -169,9 +185,9 @@ abstract contract Votes is IVotes, Context, EIP712 {
     }
 
     /**
-     * @dev "Consume a nonce": return the current value and increment.
+     * @dev Consumes a nonce.
      *
-     * _Available since v4.1._
+     * Returns the current value and increments nonce.
      */
     function _useNonce(address owner) internal virtual returns (uint256 current) {
         Counters.Counter storage nonce = _nonces[owner];

contracts/mocks/VotingMock.sol → contracts/mocks/VotesMock.sol

@@ -7,8 +7,8 @@ import "../governance/utils/Votes.sol";
 contract VotesMock is Votes {
     constructor(string memory name) EIP712(name, "1") {}
 
-    function getTotalVotes() public view returns (uint256) {
-        return _getTotalVotes();
+    function getTotalSupply() public view returns (uint256) {
+        return _getTotalSupply();
     }
 
     function delegate(address account, address newDelegation) public {
@@ -20,6 +20,6 @@ contract VotesMock is Votes {
     }
 
     function giveVotingPower(address account, uint8 amount) external {
-        _moveVotingPower(address(0), account, amount);
+        _transferVotingAssets(address(0), account, amount);
     }
 }

contracts/token/ERC721/extensions/draft-ERC721Votes.sol

@@ -7,16 +7,15 @@ import "../ERC721.sol";
 import "../../../governance/utils/Votes.sol";
 
 /**
- * @dev Extension of ERC721 to support Compound-like voting and delegation. This version is more generic than Compound's,
- * and supports token supply up to 2^224^ - 1, while COMP is limited to 2^96^ - 1.
+ * @dev Extension of ERC721 to support voting and delegation, where each individual NFT counts as 1 vote.
  *
  * This extension keeps a history (checkpoints) of each account's vote power. Vote power can be delegated either
- * by calling the {delegate} function directly, or by providing a signature to be used with {delegateBySig}. Voting
- * power can be queried through the public accessors {getVotes} and {getPastVotes}.
+ * by calling the `delegate` function directly, or by providing a signature to be used with `delegateBySig`. Voting
+ * power can be queried through the public accessors `getVotes` and `getPastVotes`.
  *
  * By default, token balance does not account for voting power. This makes transfers cheaper. The downside is that it
  * requires users to delegate to themselves in order to activate checkpoints and have their voting power tracked.
- * Enabling self-delegation can easily be done by overriding the {delegates} function. Keep in mind however that this
+ * Enabling self-delegation can easily be done by overriding the `delegates` function. Keep in mind however that this
  * will significantly increase the base gas cost of transfers.
  *
  * _Available since v4.5._
@@ -25,15 +24,15 @@ abstract contract ERC721Votes is ERC721, Votes {
     /**
      * @dev Move voting power when tokens are transferred.
      *
-     * Emits a {DelegateVotesChanged} event.
+     * Emits a {Votes-DelegateVotesChanged} event.
      */
     function _afterTokenTransfer(
         address from,
         address to,
         uint256 tokenId
     ) internal virtual override {
+        _transferVotingAssets(from, to, 1);
         super._afterTokenTransfer(from, to, tokenId);
-        _moveVotingPower(delegates(from), delegates(to), 1);
     }
 
     /**

contracts/utils/Checkpoints.sol

@@ -5,7 +5,13 @@ import "./math/Math.sol";
 import "./math/SafeCast.sol";
 
 /**
- * @dev Checkpoints operations.
+ * @dev This library defines the `History` struct, for checkpointing values as they change at different points in
+ * time, and later looking up past values by block number. See {Votes}.
+ *
+ * To create a history of checkpoints define a variable type `Checkpoints.History` in your contract, and store new
+ * checkpoints or the current transaction block using the {push} function.
+ *
+ * _Available since v4.5._
  */
 library Checkpoints {
     struct Checkpoint {
@@ -45,7 +51,10 @@ library Checkpoints {
     }
 
     /**
-     * @dev Creates checkpoint
+     * @dev Creates checkpoint if History is `empty`, or if the current `blockNumber` is higher than the latest
+     * position `_blockNumber`, otherwise will change the stored value for current block.
+     *
+     * Returns previous value and current value.
      */
     function push(History storage self, uint256 value) internal returns (uint256, uint256) {
         uint256 pos = self._checkpoints.length;
@@ -61,7 +70,7 @@ library Checkpoints {
     }
 
     /**
-     * @dev Creates checkpoint
+     * @dev Creates checkpoint using the given `op` to compute `value`.
      */
     function push(
         History storage self,

contracts/utils/README.adoc

@@ -86,6 +86,8 @@ Note that, in all cases, accounts simply _declare_ their interfaces, but they ar
 
 {{EnumerableSet}}
 
+{{Checkpoints}}
+
 == Libraries
 
 {{Create2}}

docs/modules/ROOT/pages/governance.adoc

@@ -18,10 +18,6 @@ OpenZeppelin’s Governor system was designed with a concern for compatibility w
 
 The ERC20 extension to keep track of votes and vote delegation is one such case. The shorter one is the more generic version because it can support token supplies greater than 2^96, while the “Comp” variant is limited in that regard, but exactly fits the interface of the COMP token that is used by GovernorAlpha and Bravo. Both contract variants share the same events, so they are fully compatible when looking at events only.
 
-=== ERC721Votes
-
-The ERC721 extension to keep track of votes and vote delegation is one such case.
-
 === Governor & GovernorCompatibilityBravo
 
 An OpenZeppelin Governor contract is by default not interface-compatible with GovernorAlpha or Bravo, since some of the functions are different or missing, although it shares all of the same events. However, it’s possible to opt in to full compatibility by inheriting from the GovernorCompatibilityBravo module. The contract will be cheaper to deploy and use without this module.
@@ -123,39 +119,7 @@ contract MyToken is ERC20, ERC20Permit, ERC20Votes, ERC20Wrapper {
 }
 ```
 
-If your project requires The voting power of each account in our governance setup will be determined by an ERC721 token. The token has to implement the ERC721Votes extension. This extension will keep track of historical balances so that voting power is retrieved from past snapshots rather than current balance, which is an important protection that prevents double voting.
-
-```solidity
-// SPDX-License-Identifier: MIT
-pragma solidity ^0.8.2;
-
-import "openzeppelin/contracts/token/ERC721/ERC721.sol";
-import "openzeppelin/contracts/token/ERC721/extensions/draft-ERC721Votes.sol";
-
-contract MyToken is ERC721Votes {
-    constructor() ERC721("MyToken", "MTK") EIP712("MyToken", "1") {}
-
-    // The functions below are overrides required by Solidity.
-
-    function _afterTokenTransfer(
-        address from,
-        address to,
-        uint256 tokenId
-    ) internal override(ERC721Votes) {
-        super._afterTokenTransfer(from, to, tokenId);
-    }
-
-    function _mint(address to, uint256 tokenId) internal override(ERC721Votes) {
-        super._mint(to, tokenId);
-    }
-
-    function _burn(uint256 tokenId) internal override(ERC721Votes) {
-        super._burn(tokenId);
-    }
-}
-```
-
-NOTE: Voting power could be determined in different ways: multiple ERC20 tokens, ERC721 tokens, sybil resistant identities, etc. All of these options are potentially supported by writing a custom Votes module for your Governor.
+NOTE: Voting power could be determined in different ways: multiple ERC20 tokens, ERC721 tokens, sybil resistant identities, etc. All of these options are potentially supported by writing a custom Votes module for your Governor. The only other source of voting power available in OpenZeppelin Contracts currently is xref:api:token/ERC721.adoc#ERC721Votes[`ERC721Votes`].
 
 === Governor
 

test/utils/Voting.test.js → test/governance/utils/Votes.test.js

@@ -11,7 +11,7 @@ contract('Voting', function (accounts) {
   });
 
   it('starts with zero votes', async function () {
-    expect(await this.voting.getTotalVotes()).to.be.bignumber.equal('0');
+    expect(await this.voting.getTotalSupply()).to.be.bignumber.equal('0');
   });
 
   describe('move voting power', function () {
@@ -34,8 +34,8 @@ contract('Voting', function (accounts) {
       expect(await this.voting.delegates(account3)).to.be.equal(account2);
     });
 
-    it('returns amount of votes for account', async function () {
-      expect(await this.voting.getVotes(account1)).to.be.bignumber.equal('1');
+    it('returns total amount of votes', async function () {
+      expect(await this.voting.getTotalSupply()).to.be.bignumber.equal('3');
     });
   });
 });

test/token/ERC721/extensions/ERC721Votes.test.js

@@ -453,7 +453,7 @@ contract('ERC721Votes', function (accounts) {
 
     it('returns the same total supply on transfers', async function () {
       await this.token.delegate(holder, { from: holder });
-      await this.token.delegate(recipient, { from: recipient });
+      //await this.token.delegate(recipient, { from: recipient });
 
       const { receipt } = await this.token.transferFrom(holder, recipient, NFT0, { from: holder });