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

Feature: Implements ERC1404. #15

Merged
merged 17 commits into from
Nov 28, 2019
Merged

Feature: Implements ERC1404. #15

merged 17 commits into from
Nov 28, 2019

Conversation

hickscorp
Copy link
Member

@hickscorp hickscorp commented Nov 25, 2019

This PR integrates the ERC1404 (soon to be) standard in our token.
Also, a suggestion I made to the EIP: ethereum/EIPs#1404

@hickscorp hickscorp removed the request for review from jeanparpaillon November 25, 2019 17:06
@hickscorp hickscorp force-pushed the feature/implements-ERC1404 branch 2 times, most recently from 238ae77 to eebc62d Compare November 25, 2019 17:12
import "./interfaces/IToken.sol";
import "./lib/AccountLib.sol";
import "./Registry.sol";
import "./Transact.sol";


contract Token is Initializable, IToken, IERC20 {
contract Token is Initializable, IToken, IERC20, IERC1404 {
Copy link
Member Author

Choose a reason for hiding this comment

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

@afmfe-iul This is the main change from a functional perspective. Our Token contract now has to follow that IERC1404 interface and implement the methods it imposes.

@@ -26,7 +31,21 @@ contract Token is Initializable, IToken, IERC20 {
mapping (address => AccountLib.Data) accounts;
mapping (address => mapping (address => uint256)) allowances;

// Public functions.
/// --- Constants.
Copy link
Member Author

Choose a reason for hiding this comment

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

@afmfe-iul The way ERC1404 works is that before every transfer, you call a function to "detect" (detectTransferRestriction) whether you'd be authorised to perform it. These tests should not cover balance etc, only business logic that imposes restrictions (Being an actor etc).

The detection function returns errors in the form of uint8 numbers, and exposes another function to allow a code to be mapped into a human-readable string message (messageForTransferRestriction).

These are essentially constants for each error in our transfer restriction logic, and their message counterpart.

address owner = msg.sender;
require(owner != recipient, "Recipient cannot be the same as owner");
Copy link
Member Author

@hickscorp hickscorp Nov 25, 2019

Choose a reason for hiding this comment

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

@afmfe-iul We now have a modifier (ownerAndRecipientDifferent) that does this, so we can use it here too.

// ------------------------ Public but app functions.
/// --- ERC1404 functions.

function detectTransferRestriction (address owner, address recipient, uint256) public view returns (uint8) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@afmfe-iul This is the transfer restriction detection method. Based on its input, it can return an error code or zero if everything is good.

Note that this function shall be called from the front-end wallet in the next iteration, so the results are consistent with the ERC1404 implementation.
Basically when a user would push the "Transfer" button in the front-end, we would run the detection function, and if the return code isn't zero we would display a message explaining that a restriction prevents the transfer to be initiated. Since the function is a view (not a transaction), it can be called for free.

}
}

function messageForTransferRestriction (uint8 errCode) public view returns (string memory) {
Copy link
Member Author

@hickscorp hickscorp Nov 25, 2019

Choose a reason for hiding this comment

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

@afmfe-iul This function converts restriction detection error codes into human-readable messages. From a front-end perspective they will be useful if the detection function triggers to retrieve an error message that we can display as an explanation.

@@ -228,10 +283,26 @@ contract Token is Initializable, IToken, IERC20 {
_;
}

modifier isActor(address c) {
modifier isOwnerActor(address c) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@afmfe-iul I decided to remove the plain isActor modifier in favour of more verbose ones (so you can know which one of your parameter is actually not an actor).

@@ -0,0 +1,26 @@
pragma solidity ^0.5.9;
Copy link
Member Author

Choose a reason for hiding this comment

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

@afmfe-iul This is the official ERC1404 interface as described in the EIP that can be found here. It is not closed but I can't forsee major changes to it so it's safe to assume that it will remain identical for the main things.

@tableturn tableturn deleted a comment from codecov bot Nov 25, 2019
@codecov
Copy link

codecov bot commented Nov 25, 2019

Codecov Report

Merging #15 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #15   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          17     17           
  Lines         296    315   +19     
  Branches       32     38    +6     
=====================================
+ Hits          296    315   +19
Impacted Files Coverage Δ
contracts/Token.sol 100% <100%> (ø) ⬆️

@hickscorp hickscorp merged commit 3795613 into master Nov 28, 2019
@hickscorp hickscorp deleted the feature/implements-ERC1404 branch November 28, 2019 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant