Clarify event and comments (#14)

Author: Adrien Hamelin

contracts/mocks/ERC721RentAgreementMock.sol

@@ -8,7 +8,7 @@ contract ERC721RentAgreementMock is IERC721RentalAgreement {
     bool private _fail;
 
     // Interface
-    function afterRentalAgreementReplaced(uint256) external view override {
+    function afterAgreementRemoved(uint256) external view override {
         require(!_fail, "Failed from agreement contract");
     }
 

contracts/token/ERC721/ERC721.sol

@@ -203,7 +203,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Rental, IERC721Metadata {
         _rentalAgreements[tokenId] = agreement;
 
         if (address(currentAgreement) != address(0)) {
-            currentAgreement.afterRentalAgreementReplaced(tokenId);
+            currentAgreement.afterAgreementRemoved(tokenId);
         }
     }
 
@@ -411,7 +411,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Rental, IERC721Metadata {
         delete _rentalAgreements[tokenId];
 
         if (address(rentAgreement) != address(0)) {
-            rentAgreement.afterRentalAgreementReplaced(tokenId);
+            rentAgreement.afterAgreementRemoved(tokenId);
         }
 
         emit Transfer(from, to, tokenId);

contracts/token/ERC721/ERC721BundleRentalAgreement.sol

@@ -41,7 +41,7 @@ contract ERC721BundleRentalAgreement is IERC721RentalAgreement, ERC165 {
     }
 
     /// @inheritdoc IERC721RentalAgreement
-    function afterRentalAgreementReplaced(uint256 tokenId) external view virtual override {
+    function afterAgreementRemoved(uint256 tokenId) external view virtual override {
         // We don't need to check if a rental is in progress because the IERC721Rental does that for us
         // We don't allow to change the contract if funds remain, because we don't store the owner
         require(

contracts/token/ERC721/ERC721SingleRentalAgreement.sol

@@ -77,7 +77,7 @@ contract ERC721SingleRentalAgreement is Context, IERC721RentalAgreement, ERC165
     }
 
     /// @inheritdoc IERC721RentalAgreement
-    function afterRentalAgreementReplaced(uint256) public view override onlyErc721Contract {
+    function afterAgreementRemoved(uint256) public view override onlyErc721Contract {
         require(
             rentalStatus == RentalStatus.pending,
             "ERC721SingleRentalAgreement: rental agreement has to be pending to be updated."

contracts/token/ERC721/ERC721SwapRentalAgreement.sol

@@ -70,7 +70,7 @@ contract ERC721SwapRentalAgreement is Context, IERC721RentalAgreement, ERC165 {
     }
 
     /// @inheritdoc IERC721RentalAgreement
-    function afterRentalAgreementReplaced(uint256) public view override onlyErc721Contracts {
+    function afterAgreementRemoved(uint256) public view override onlyErc721Contracts {
         require(
             rentalAgreement.rentalStatus == RentalStatus.pending,
             "ERC721SwapRentalAgreement: rental agreement already active"

contracts/token/ERC721/extensions/IERC721Rental.sol

@@ -9,14 +9,14 @@ import "../../../utils/introspection/IERC165.sol";
 /// Defines the interface that a rental agreement contract should support to be used by
 /// `IERC721Rental`.
 interface IERC721RentalAgreement is IERC165 {
-    /// Function called at the end of `IERC721Rental.setRentalAgreement` to replace an
-    /// existing rental agreement contract of a token.
+    /// Function called at the end of `IERC721Rental.setRentalAgreement` on the agreement
+    /// currently set for the token, if one exists.
     ///
     /// @dev Allows the agreement to cancel the change by reverting if it deems it
     /// necessary. The `IERC721Rental` is calling this function, so all information needed
-    /// can be queried through the `msg.sender`. This event is not called if a rental agreement
-    /// is not in progress.
-    function afterRentalAgreementReplaced(uint256 tokenId) external;
+    /// can be queried through the `msg.sender`. This event is only called when the token
+    /// is not rented, as it is not allowed to change an agreement during a rental.
+    function afterAgreementRemoved(uint256 tokenId) external;
 
     /// Function called at the end of `IERC721Rental.acceptRentalAgreement`.
     ///
@@ -52,13 +52,13 @@ interface IERC721RentalAgreement is IERC165 {
 interface IERC721Rental is IERC721 {
     /// Set the rental agreement contract for a specific token.
     ///
-    /// A previously set rental agreement contract must accept its replacement.
-    /// The caller must be the owner or their approver or operator.
+    /// A previously set rental agreement contract must accept the change.
+    /// The caller must be the owner (not the renter) or their approver or operator.
     /// The token must not be currently rented.
     /// The agreement is removed upon token transfer.
     ///
-    /// @dev If an agreement was set, calls its
-    /// `IERC721RentalAgreement.afterRentalAgreementReplaced` at the end of the call.
+    /// @dev If an agreement was already set before this call, calls its
+    /// `IERC721RentalAgreement.afterAgreementRemoved` at the end of the call.
     ///
     /// @param agreement The agreement. Set to 0 to remove the current agreement.
     function setRentalAgreement(IERC721RentalAgreement agreement, uint256 tokenId) external;
@@ -87,7 +87,7 @@ interface IERC721Rental is IERC721 {
     /// @dev Calls `IERC721RentalAgreement.afterRentalStopped` at the end of the call.
     function stopRentalAgreement(uint256 tokenId) external;
 
-    /// @return The address of the rented token owner, or 0 is there is no rental in
-    /// progress.
+    /// @return The address of the asset owner and not the renter, or 0 if there is no
+    /// rental in progress.
     function rentedOwnerOf(uint256 tokenId) external view returns (address);
 }

test/token/ERC721/ERC721BundleRentAgreement.test.js

@@ -38,7 +38,7 @@ contract('ERC721BundleRentalAgreement', function (accounts) {
     it('cannot call IERC721RentalAgreement to modify state', async function () {
       expect(
         ERC721BundleRentalAgreement.abi.find(
-          (abi) => abi.name === 'afterRentalAgreementReplaced',
+          (abi) => abi.name === 'afterAgreementRemoved',
         ).constant,
       ).to.equal(true);
       expect(

test/token/ERC721/ERC721SingleRentalAgreement.test.js

@@ -53,13 +53,13 @@ contract('ERC721SingleRentalAgreement', function (accounts) {
 
     it('Only erc721 contract can update state', async function () {
       await expectRevert(
-        this.erc721SingleRentalAgreement.afterRentalAgreementReplaced(this.tokenId, { from: this.renter }),
+        this.erc721SingleRentalAgreement.afterAgreementRemoved(this.tokenId, { from: this.renter }),
         'ERC721SingleRentalAgreement: only erc721Contract contract can modify rental agreement state',
       );
     });
 
     it('Enable to change agreement when status is pending', async function () {
-      await this.erc721SingleRentalAgreement.afterRentalAgreementReplaced(this.tokenId, { from: this.erc721.address });
+      await this.erc721SingleRentalAgreement.afterAgreementRemoved(this.tokenId, { from: this.erc721.address });
     });
   });
 
@@ -94,7 +94,7 @@ contract('ERC721SingleRentalAgreement', function (accounts) {
       // Pay rent.
       await this.erc721SingleRentalAgreement.payAndStartRent({ from: this.renter, value: this.rentalFees });
       await expectRevert(
-        this.erc721SingleRentalAgreement.afterRentalAgreementReplaced(this.tokenId, { from: this.erc721.address }),
+        this.erc721SingleRentalAgreement.afterAgreementRemoved(this.tokenId, { from: this.erc721.address }),
         'ERC721SingleRentalAgreement: rental agreement has to be pending to be updated',
       );
     });