ZRC-2 - Fungible Tokens standard #31

snowsledge · 2020-01-06T11:24:08Z

Implementation of Fungible Tokens standard in Scilla.

Reference contract: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC777/ERC777.sol

AmritKumar · 2020-01-06T23:56:47Z

@vaivaswatha Can you take a look at it too?

vaivaswatha · 2020-01-07T04:40:51Z

Does this PR mean that Create FungibleToken.scilla #26 will be discarded?
The spec here ZRC-2: Fungible Token #25 has removed the concept of default operators, but the implementation has it.

snowsledge · 2020-01-07T05:44:47Z

Yes, Create FungibleToken.scilla #26 will be discarded. This will replace that PR.
That is a conflict that is resolved in this PR too. default_operator role is resumed in both tech spec and reference contract implementation.

vaivaswatha · 2020-01-07T05:48:25Z

Yes, Create FungibleToken.scilla #26 will be discarded. This will replace that PR.

That is a conflict that is resolved in this PR too. default_operator role is resumed in both tech spec and reference contract implementation.

Can you link to the spec that I must refer to, which is compatible with this PR ?

snowsledge · 2020-01-07T05:55:11Z

This is the spec link which is in this PR:
https://github.com/Zilliqa/ZRC/blob/258144731392ca7acfda17aa106171be189e3b48/zrcs/zrc-2.md

The reference ERC-777 contract is located here:
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC777/ERC777.sol

zrcs/zrc-2.md

reference/FungibleToken.scilla

vaivaswatha · 2020-01-07T10:50:23Z

reference/FungibleToken.scilla

+      can_do = uint128_le amount min;
+      match can_do with
+      | True =>
+        new_from_bal = builtin sub bal amount;


Can you not call ProcedureMove here?

Due to this min = min_int bal allowed; requirement, we cannot call ProcedureMove as it takes only balance into account.

reference/FungibleToken.scilla

…eOperator with RevokedDefaultOperator transitions

teye · 2020-01-15T08:04:20Z

reference/FungibleToken.scilla

+
+(* @dev: Revoke an address from being an operator or default_operator of the caller. *)
+(* @param operator: Address to be removed as operator or default_operator.           *)
+transition RevokeOperator(operator: ByStr20)


For AuthorizeOperator there is a check to see if the sender is trying to add themselves into the authorized list. However for the RevokeOperator, there is no such check if the sender is trying to add themselves into the revoked list?

Good suggestion, adding the check for self.

anton-trunov

LGTM
I've proposed a few suggestions, though.

anton-trunov · 2020-01-15T09:50:30Z

reference/FungibleToken.scilla

+      match result with
+      | CodeNotAuthorized    => Int32 -1
+      | CodeNotFound         => Int32 -2
+      | CodeNotAllowed      => Int32 -3


Suggested change

| CodeNotAllowed => Int32 -3

| CodeNotAllowed => Int32 -3

anton-trunov · 2020-01-15T09:59:54Z

reference/FungibleToken.scilla

+field balances_map: Map ByStr20 Uint128 
+  = Emp ByStr20 Uint128
+
+field operators_map: Map ByStr20 (Map ByStr20 Bool) 


I would redefine the type of operators_map to Map ByStr20 (Map ByStr20 Unit), where Unit is defined as follows: type Unit = | Unit. This clearly indicates that Map ByStr20 Unit is a set, i.e. we don't care about the value stored at a particular address, but only about the presence of the address. Whereas I find using Bool type (and only one its value True) for that a bit confusing.

The same applies to revoked_default_operators down below.

anton-trunov · 2020-01-15T10:13:16Z

reference/FungibleToken.scilla

+(* Procedures *)
+
+(* Emit Errors *)
+procedure MakeErrorEvent(err : Error)


Suggested change

procedure MakeErrorEvent(err : Error)

procedure IssueErrorEvent(err : Error)

We already have make_error_event. The name suggestion here stresses that it's an action. PublishErrorEven or something like that would do too. Note that I didn't put suggestions on the call sites of MakeErrorEvent.

anton-trunov · 2020-01-15T10:17:48Z

reference/FungibleToken.scilla

+    match get_bal with
+    | Some bal => bal
+    | None => Uint128 0
+    end;


Looks like this can be factored out as a function. It's also used in BalanceOf transition.

anton-trunov · 2020-01-15T10:39:52Z

reference/FungibleToken.scilla

+end
+
+(* Mint Tokens *)
+procedure ProcedureMint(recipient: ByStr20, amount: Uint128) 


Suggested change

procedure ProcedureMint(recipient: ByStr20, amount: Uint128)

procedure AuthorizedMint(recipient: ByStr20, amount: Uint128)

anton-trunov · 2020-01-15T13:09:04Z

reference/FungibleToken.scilla

+  is_operator_approved = 
+    match get_operator with
+    | None => False
+    | Some status => status
+    end;


This block is not needed if the suggestion above is accepted.

anton-trunov · 2020-01-15T13:12:07Z

reference/FungibleToken.scilla

+  end
+end
+
+(* @dev: Returns the number of tokens an approved_spender is allowed to spend on behalf of the token_owner. *)


"Returns" here is a bit confusing.

anton-trunov · 2020-01-15T13:13:00Z

reference/FungibleToken.scilla

+end
+
+
+(* @dev: Returns the total amount of tokens in existence. *)


just the same as for "Returns" above

anton-trunov · 2020-01-15T13:13:19Z

reference/FungibleToken.scilla

+  event e
+end
+
+(* @dev: Returns the amount of tokens owned by an address. *)


"Returns" too

anton-trunov · 2020-01-15T13:14:13Z

reference/FungibleToken.scilla

+  balance = 
+    match get_bal with
+    | Some bal => bal
+    | None => Uint128 0
+    end;


Candidate to be replace by a function call as in an analogous suggestion at the beginning of the contracts

snowsledge added 2 commits January 6, 2020 19:17

feat: ZRC-2 Fungible Tokens Standard

2581447

fix: Correct contributors for ZRC-1 and ZRC-2

4ad0a2b

snowsledge requested review from anton-trunov, AmritKumar, vaivaswatha and edison0xyz January 6, 2020 11:24

snowsledge requested a review from jjcnn as a code owner January 6, 2020 11:24

fix: OperatorSend transition logic fix

4ff8f81

vaivaswatha reviewed Jan 7, 2020

View reviewed changes

zrcs/zrc-2.md Outdated Show resolved Hide resolved

fix: TransferFrom transition logic fix

936ba05

vaivaswatha reviewed Jan 7, 2020

View reviewed changes

snowsledge added 2 commits January 7, 2020 22:31

feat: Merge AuthorizeOperator with ReAuthorizeDefaultOperator & Revok…

7bc57be

…eOperator with RevokedDefaultOperator transitions

feat: Simplify TransferFrom transition

e91e228

vaivaswatha approved these changes Jan 8, 2020

View reviewed changes

fix: Use IntUtils library function uint128_le instead

ed7fcad

snowsledge mentioned this pull request Jan 8, 2020

Add credits #28

Closed

GreyEcologist mentioned this pull request Jan 8, 2020

ZRC#2 #24

Open

snowsledge requested a review from teye January 14, 2020 02:54

teye reviewed Jan 15, 2020

View reviewed changes

anton-trunov mentioned this pull request Jan 15, 2020

[Gas-Analysis] Model folds correctly Zilliqa/scilla#756

Open

anton-trunov approved these changes Jan 15, 2020

View reviewed changes

fix: Address Anton and Teye comments

7e4aa68

snowsledge merged commit f9d07dd into master Jan 16, 2020

teye approved these changes Jan 16, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ZRC-2 - Fungible Tokens standard #31

ZRC-2 - Fungible Tokens standard #31

snowsledge commented Jan 6, 2020

AmritKumar commented Jan 6, 2020

vaivaswatha commented Jan 7, 2020

snowsledge commented Jan 7, 2020

vaivaswatha commented Jan 7, 2020

snowsledge commented Jan 7, 2020

vaivaswatha Jan 7, 2020

snowsledge Jan 7, 2020

teye Jan 15, 2020

snowsledge Jan 16, 2020

anton-trunov left a comment

anton-trunov Jan 15, 2020

anton-trunov Jan 15, 2020

anton-trunov Jan 15, 2020

anton-trunov Jan 15, 2020

anton-trunov Jan 15, 2020

anton-trunov Jan 15, 2020

anton-trunov Jan 15, 2020

anton-trunov Jan 15, 2020

anton-trunov Jan 15, 2020

anton-trunov Jan 15, 2020

	procedure MakeErrorEvent(err : Error)
	procedure IssueErrorEvent(err : Error)

	procedure ProcedureMint(recipient: ByStr20, amount: Uint128)
	procedure AuthorizedMint(recipient: ByStr20, amount: Uint128)

		end


		(* @dev: Returns the total amount of tokens in existence. *)

ZRC-2 - Fungible Tokens standard #31

ZRC-2 - Fungible Tokens standard #31

Conversation

snowsledge commented Jan 6, 2020

AmritKumar commented Jan 6, 2020

vaivaswatha commented Jan 7, 2020

snowsledge commented Jan 7, 2020

vaivaswatha commented Jan 7, 2020

snowsledge commented Jan 7, 2020

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

anton-trunov left a 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

Choose a reason for hiding this comment