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

UpdateERCs #6

Merged
merged 5 commits into from
Dec 10, 2018
Merged

UpdateERCs #6

merged 5 commits into from
Dec 10, 2018

Conversation

adamdossa
Copy link
Collaborator

No description provided.

eip/eip-1400.md Outdated

## Motivation

Accelerate the issuance and management of securities on the Ethereum blockchain by specifying a standard interface through which security tokens can be operated on and interrogated by all relevant parties.

Security tokens differ materially from other token use-cases, with more complex interactions between off-chain and on-chain actors, and considerable regulatory scrutiny.
The security token standard (#1594) provides document management, error signalling, gate keeper (operator) access control, off-chain data injection and issuance / redemption semantics.

Choose a reason for hiding this comment

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

These could potentially be split into individual eips. For example, the scope of operations reserved for the controller (minting, forced transfers) can be an extension to a basic controller eip, this would allow for some implementations to use the same controller framework with different operations.

Choose a reason for hiding this comment

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

ERC-1400 Family of Standards:

  1. Extended ERC-20 (transferWithData, mint/burn)
  2. Document Management Standard
  3. Error Signalling Standard / Transfer Restrictions
  4. Controller Permission Management Standard (log changes in permissioned operations + addresses, query permissioned addresses)
  5. Differentiated Ownership Standard
  6. Cashflow Management Standard
  7. Asset Composability Standard
  8. Corporate Action Standard

eip/eip-1410.md Outdated
function operatorTransferByTranche(bytes32 _tranche, address _from, address _to, uint256 _amount, bytes _data, bytes _operatorData) external returns (bytes32);
```

#### canTransferByTranche

Choose a reason for hiding this comment

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

Should the canTransferByTranche be a optional requirement?

I am not familiar enough with the use cases outside of permissioned tokens to know.

eip/eip-1594.md Outdated

This standard represents a decomposition of ERC-1400 (#1411) to split out the tranche functionality (ERC-1410 #1410) from the remainder of the security token functionality.

It further updates ERC-1400 (#1411) to:

Choose a reason for hiding this comment

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

I've come to think that forced transfers should be split out into dedicated interface due to transparency and reporting requirements.

This would mean the following changes:

  1. Dedicated function for forced transfer execution with event.
  2. Getter function to get addresses which are allowed to perform forced transfers. (can extend the function to check if forced transfers are allowed).
  3. Event to notify when there is a change in addresses with forced transfer permissions.

eip/eip-1400.md Outdated
function operatorRedeemByPartition(bytes32 _partition, address _tokenHolder, uint256 _amount, bytes _operatorData) external;

// Transfer Validity
function canTransfer(address _from, address _to, uint256 _amount, bytes _data) external view returns (byte, bytes32);
Copy link

Choose a reason for hiding this comment

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

Suggested change
function canTransfer(address _from, address _to, uint256 _amount, bytes _data) external view returns (byte, bytes32);
function canTransfer(address _to, uint256 _value) external view returns (bool, bytes1);
function canTransferFrom(address _from, address _to, uint256 _value) external view returns (bool, bytes1);
function canTransferWithData(address _to, uint256 _value, bytes _data) external view returns (bool, bytes1);
function canTransferFromWithData(address _from, address _to, uint256 _value, bytes _data) external view returns (bool, bytes1);

Rationale:

  • Including a canTransferFrom check enables checks for the validity of the operator and whether sufficient allowances are set.
  • Breaking out the bytes _data parameter into canTransferWithData and canTransferFromWithData will lead to smoother integrations in cases with no data (which seem to be the vast majority as of now) but will not incur much implementation penalty as the same internal functions can be reused across each.
  • Returning a bool is a recurrent request among implementors, and will be the main variable that many integrations are actually checking for. Surfacing the reason via a byte (named bytes1 here to be more explicit) is still important so as to conform with EIP-1066. The additional bytes32 status could still be included as a third return value if necessary, but an optional, more verbose interface could be provided (my preferred solution).
  • Setting parameter names to directly shadow the actual ERC20 transfer methods makes the association more clear.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've updated to split out canTransfer from canTransferFrom and added a bool return value. I haven't modified to add different versions for withData - my preference would be to have a single version, but effectively pass in an empty _data value if not appropriate. I'll add to the agenda for the next community call.

| `0x56` | invalid sender |
| `0x57` | invalid receiver |
| `0x58` | invalid operator (transfer agent) |
| `0x59` | |
Copy link

Choose a reason for hiding this comment

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

Suggested change
| `0x59` | |
| `0x59` | invalid balance |

This code is important for identifying cases where minimum or maximum balance requirements would be violated. Many regulations and other agreements will require that ownership is not concentrated too heavily.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we are going to make some standardization over the metadata of the securities then we also need invalid data error code


Security token contracts can reference this metadata in order to apply additional logic to determine whether or not a transfer is valid, and determine the metadata that should be associated with the tokens once transferred into the receiver's balance.

To represent securities metadata we use the ERC 1410 (#1410) - Partially Fungible Token Standard.
To represent securities metadata we use ERC-1410 (#1410) - Partially Fungible Token Standard.

Transfers of securities can fail for a variety of reasons in contrast to utility tokens which generally only require the sender to have a sufficient balance.

These conditions could be related to metadata of the securities being transferred (i.e. whether they are subject to a lock-up period), the identity of the sender and receiver of the securities (i.e. whether they have been through a KYC process, whether they are accredited or an affiliate of the issuer) or for reasons unrelated to the specific transfer but instead set at the token level (i.e. the token contract enforces a maximum number of investors or a cap on the percentage held by any single investor).

Copy link
Collaborator

Choose a reason for hiding this comment

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

For metadata, do we have the specific format related to securities as ERC1410 can be used in other use cases as well. So I think for securities we need to create a Specific format of metadata that makes easier to read the values of it on-chain & off-chain.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed. Need to think about where this should live though as ERC 1410 is not security token specific. Probably as a part of ERC 1400.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes I am more inclined that it will live in the ERC1400


#### operatorRedeemByTranche
If the token is controllable (`isControllable` returns `TRUE`) then the controller may use `operatorTransferByPartition` without being explicitly authorised by the token holder.
Copy link
Collaborator

Choose a reason for hiding this comment

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

On the literal side, Controllable and operator are not making a good pair,
I prefer to make isControllable() => isOperatable() that clearly defines the operator can transfer the tokens. because on the literal side isControllable() is not meant to allow the transfer for the operator. Or we can replace the operator to controller in the function name.

We could also change the function name operatorTrasferByPartition to transferPartitionByOperator. I think later makes more sense to facilitate the transfer by the operator.

Choose a reason for hiding this comment

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

If there are provisions for operators of specific partitions why not match this domain to isControllable, i.e. isControllable(bytes32 partition) ?

``` solidity
function operatorRedeemByTranche(bytes32 _tranche, address _tokenHolder, uint256 _amount, bytes _operatorData) external;
```
### operatorRedeemByPartition
Copy link
Collaborator

Choose a reason for hiding this comment

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

same for this name redeemPartitionByOperator

eip/eip-1400.md Outdated
address controller,
address indexed from,
address indexed to,
uint256 amount,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Then amount should be replaced to _value to make ERC20 compatible

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I switched to using value rather than amount everywhere for consistency.

eip/eip-1410.md Outdated

This function MUST emit a `TransferByPartition` event for successful transfers.

This function MUST emit a `ChangedPartition` event if the partition of the receiver differs to the sender.
Copy link
Collaborator

@satyamakgec satyamakgec Dec 3, 2018

Choose a reason for hiding this comment

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

I am a little bit skeptical about the emission of ChangedPartition event.

On the sender's side, there is no change in the partition whether it sends some token from a partition or send all tokens from a partition (give zero when calling balanceOfByPartition()).

On the receiver's side, whether it adds the token into the existing partition(that may or may not be the same as the sender's but still there is no change of the partitions on the receiver's side) or make tokens non-fungible (0x0 value for the partition, if we are allowing that otherwise, it will take some default partition value).

We already emitting and returning the receiver partition. That allows the application to know about the change by comparing the sender's and receiver's partition.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The use-case I was thinking about was if a users tokens moved from partition to another partition (i.e. from unlocked to locked) as part of another operation (not necessarily a transfer). I agree though that this may not always be reasonable, especially if the partition balances are determined programatically rather than as explicitly stored values.

The function MUST return the `bytes32 _tranche` of the receiver.
``` solidity
function transferByPartition(bytes32 _partition, address _to, uint256 _amount, bytes _data) external returns (bytes32);
```
Copy link
Collaborator

@satyamakgec satyamakgec Dec 3, 2018

Choose a reason for hiding this comment

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

is there any specific reason to not returning the ESC here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It would throw (revert) on failure. The canTransfer... functions can be used if a more informative response is needed, but I think actual transfer functions need to revert if they fail.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Make sense

eip/eip-1594.md Outdated
`setDocument` MUST emit a `Document` event with details of the document being attached or modified.

``` solidity
function getDocument(bytes32 _name) external view returns (string, bytes32);

Choose a reason for hiding this comment

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

I've received a suggestion to return a timestamp of the latest edit transaction for the document.

Choose a reason for hiding this comment

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

There should also be a separate getter which returns an array of bytes32 name used by the implementation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added this.

@ijxy
Copy link

ijxy commented Dec 6, 2018

Hi guys, thanks for the great work on this. It's just what we're crying out for in the ecosystem right now.

I've noticed you haven't added ERC165 compatibility to the standard and was wondering whether there was any reason why, or whether it could be added? As you probably know, it is not part of ERC20 but is part of ERC721. I think it would be a useful addition looking ahead to Ethereum security token platforms.

function authorizeOperatorByPartition(bytes32 _partition, address _operator) external;
function revokeOperatorByPartition(bytes32 _partition, address _operator) external;

// Operator Information
Copy link

@nevillegrech nevillegrech Dec 8, 2018

Choose a reason for hiding this comment

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

I think the API can be simplified. Most operations exist in two states verbNoun(...) and verbNounByPartition(partition, ...), e.g., authorizeOperator, authorizeOperatorByPartition, revokeOperator, revokeOperatorByPartition, isOperator, isOperatorForPartition, etc..

Instead, I suggest keeping only one version (with the partition) for functions and events. If needed, the implementor can specify a special partition that represents all partitions (top). I would not over-specify this functionality (e.g., define hierarchies of partitions), to allow the implementers to implement any logic they need to simplify the regulatory compliance in their specific jurisdiction.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hey @nevillegrech - I agree with a lot of this. One issue is that we wanted to make this part of the standard separate to the other core security token functionality (by splitting it into #1410) which implies a different naming standard. As / when standardisation and consensus changes though this is def. a simpler approach to bear in mind.

@adamdossa
Copy link
Collaborator Author

Hi guys, thanks for the great work on this. It's just what we're crying out for in the ecosystem right now.

I've noticed you haven't added ERC165 compatibility to the standard and was wondering whether there was any reason why, or whether it could be added? As you probably know, it is not part of ERC20 but is part of ERC721. I think it would be a useful addition looking ahead to Ethereum security token platforms.

I like ERC-165. I think adding support for this could certainly be part of the next updates for these standards if there is consensus on this.

@adamdossa
Copy link
Collaborator Author

@channel - I have updated the PR to reflect a lot of the discussion / comments. I think at this point, unless someone feels very strongly I'll draw a line under this set of updates, and then raise the open questions discussed above as part of the agenda for the next community call.

@ijxy
Copy link

ijxy commented Dec 11, 2018

@adamdossa Where would you like feedback to be posted to?

For one thing, I think having smaller, separate interfaces is a much better idea than the mono(lithic)-interface.

However, there are some things I would change with certain call signatures. For example, I would prefer someFuncByPartition(..., bytes32 _partition) which I feel is much more intuitive.

I also agree with the other comment made earlier than operatorX should be xByOperator etc.

Finally, I'm actually wondering what the point of the operator actually is. Isn't the approve mechanism sufficient? What can an operator do that an approved account can't?

Really exciting stuff, this feels really close to a realistic, usable offering now!

@adamdossa
Copy link
Collaborator Author

@ParkinsonJ probably the best thing would be to open these as issues in this repo. I'll go through this PR and create issues from the comments etc. not addressed.

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.

6 participants