Add KEEP token contract 🚀 by ngrinkevich · Pull Request #10 · threshold-network/keep-core

ngrinkevich · 2017-11-16T18:40:44Z

Support for vesting grants
Support for staking. Include a collection of each staker's public address and how much they've staked.
Support for staking vested balances
Withdrawal delay for staking
Withdrawal delay for staking vested tokens
Revoke for vesting
Solidity unit tests
Truffle e2e tests for contracts
Make it easy / efficient to refer to a staker outside the staking contract

mhluongo · 2017-11-17T05:39:09Z

+import 'zeppelin-solidity/contracts/token/MintableToken.sol';
+
+contract KeepToken is MintableToken {
+  string public name = "KEEP TOKEN";


Should this be all caps?

mhluongo · 2017-11-20T03:53:58Z

@@ -0,0 +1,50 @@
+pragma solidity ^0.4.18;
+
+import 'zeppelin-solidity/contracts/token/MintableToken.sol';


Do we want this to be mintable if it has a fixed supply?

yeah I think it may put off some investors knowing that more tokens can be issued, I'll remove

mhluongo · 2017-11-20T03:58:16Z

+
+  mapping(address => uint256) stakeBalances;
+
+  event StakeIn(address indexed from, uint256 value);


I think Stake and Unstake might make more sense as event names?

mhluongo · 2017-11-20T04:01:32Z

+  function stakeOut(uint256 _value) public returns (bool) {
+    require(_value <= stakeBalances[msg.sender]);
+
+    stakeBalances[msg.sender] = stakeBalances[msg.sender].sub(_value);


We want a configurable withdrawal delay for staking, ideally in a separate, reusable token contract that KEEP can inherit from.

Eg, StakableToken, with a function initiateUnstake that starts the delay timer, and another, finishUnstake... or similar. Thoughts on terminology @Shadowfiend?

finishUnstake would be internal or invokable on-chain? The terminology makes sense to me in general, assuming I've correctly understood that this is to implement the functionality for someone trying to wind down their participation in the relay/keep network.

mhluongo · 2017-11-20T04:02:26Z

+  * @param _value The amount to be staked in
+  */
+  function stakeIn(uint256 _value) public returns (bool) {
+    require(_value <= balances[msg.sender]);


What if msg.sender has no balance?

(Easy to imagine the mapping defaults uint256s to 0, just confirming)

yep, defaults to 0
Mappings can be seen as hash tables which are virtually initialized such that every possible key exists and is mapped to a value whose byte-representation is all zeros.
from http://solidity.readthedocs.io/en/develop/introduction-to-smart-contracts.html

mhluongo · 2017-11-20T04:13:26Z

+  string public symbol = "KEEP";
+  uint256 public decimals = 18;
+
+  mapping(address => uint256) stakeBalances;


This won't work with https://github.com/OpenZeppelin/zeppelin-solidity/blob/master/contracts/token/TokenVesting.sol as is

mhluongo · 2017-11-20T04:14:32Z

Oh- and tests! Tests, tests, tests!

mhluongo · 2017-11-20T04:16:45Z

Benefit of separating any token changes into their own contract that we inherit from- we can submit them upstream if they're generally useful.

ngrinkevich · 2017-11-24T14:07:30Z

Benefit of separating any token changes into their own contract that we inherit from- we can submit them upstream if they're generally useful.

yep, splitting Staking into separate stakable.sol looks easy enough, just need to add configurable withdrawal.

Tricky part is how to neatly plugin vesting functionality, but I'm sure I'll figure it out pretty soon )

Shadowfiend · 2017-11-29T15:06:22Z

Just to check, is our goal here a long-term PR that will eventually merge the full Solidity token contract, or is the goal rather to do small PRs that slowly move us closer to the overall token contract?

ngrinkevich · 2017-11-29T15:12:53Z

@Shadowfiend long-term PR I think suits better here if thats ok. Code may change quite a bit in the process. It's not straight forward development as I initially thought :)

Shadowfiend · 2017-11-29T15:26:31Z

I'm down with that. It may help to add some checkboxes to the description with the milestones we're trying to hit in that case, so we can have a clear sense of where we are with respect to our final merge goals.

ngrinkevich · 2017-11-29T15:33:35Z

It may help to add some checkboxes to the description with the milestones we're trying to hit in that case, so we can have a clear sense of where we are with respect to our final merge goals.

good idea! updated

Shadowfiend · 2017-11-29T15:55:45Z

🙇

mhluongo · 2017-12-10T06:02:23Z

@ngrinkevich can you put your PGP key on GH? I think you have code signing but no verified key on your account

mhluongo · 2017-12-10T06:22:41Z

Overarching concern here- we want to be able to upgrade the staking contract without upgrading the token contract (as upgrading the token contract could mean the team can inflate the supply, etc). That means breaking staking / vesting out of the token contract and into their own

ngrinkevich · 2017-12-10T17:34:51Z

That means breaking staking / vesting out of the token contract and into their own

yeah I agree if we plan upgrades that definitely a must
good news I've been working on solutions already will show something on Monday

mhluongo · 2017-12-10T18:07:59Z

Awesome. I've also started thinking about how the threshold relay verification and MPC can slash or redistribute stakes when people misbehave- when we add that to staking we'll need a way to set the "stake controller" contract address that's distinct from the owner. That contract will be allowed to "burn" staked coins, and move staked coins between addresses. I figure we close out this PR before we worry about that, but your call

ngrinkevich · 2017-12-11T16:52:18Z

Nearly done on the contracts split up, quick info on this:

Initially I thought staking contract can call KEEP token transfer() on behalf of the user who calls staking contract, but turned out can't use msg.sender since in that case it becomes staking contract address, some examples on the internet use tx.origin instead which represents the initial user in this chain call, but at the same time some say its considered bad practice and that tx.origin somehow can be spoofed.

So seems like we’re back to 2 steps process, we call "approve” step on KEEP token first and then stake, i.e.

user call token contract approve() function with the amount and the address of the staking contract
user call stake() on staking contract with the amount, we then do aditional checks for approved amount and modify the balances etc...

There is a known issue with this approve/transferFrom https://docs.google.com/document/d/1YLPtQxZu1UAvO9cZ1O2RPXBbT0mooh4DYKjA_jp-RLM/edit#heading=h.m9fhqynw2xvt
but looks like easily can be avoided by reducing the spender's allowance to 0 and set the desired value afterwards also another way to mitigate the threat is

to approve token transfers only to smart contracts with verified source code that does not contain logic for performing attacks like described above, and to accounts owned by the people you may trust.

mhluongo · 2018-01-24T17:07:59Z

Let's get this out. @Shadowfiend what do you think about Nik presenting this to the rest of the team on a call and doing one last round of review?

Shadowfiend · 2018-01-30T19:07:24Z

@mhluongo Yes 👍

Shadowfiend · 2018-01-30T19:08:28Z

+      gas: 6000000
+    },
+    testnet: {
+      host: "10.51.244.207",


Seems like something we won't want in a public repo… If that's correct, let's:
(a) change the IP and
(b) use a hostname

If that's not correct, then never mind :)

Shadowfiend · 2018-02-05T20:50:36Z

One note here: we've discussed the staking registry containing a public key. We'll need to either include that in the staking contract, or maintain a separate registry address => public key somewhere that references the staking registry, I guess?

mhluongo · 2018-02-06T02:41:04Z

Added an issue

Shadowfiend · 2018-02-07T15:13:06Z

@ngrinkevich I think all I'm waiting on to approve here is a response on my last note. If you can get to that before anything else, we can merge this PR and start basing stuff (e.g. the pubkey/stake registry work) on master.

Shadowfiend · 2018-02-16T15:33:57Z

+   * @param _extraData Extra information to send to the approved contract.
+  */
+  function approveAndCall(address _spender, uint256 _value, bytes _extraData) public returns (bool success) {
+    tokenRecipient spender = tokenRecipient(_spender);


Just to double-check here, there's no guarantee that _spender will actually conform to tokenRecipient, right? Is there a danger anyone can do anything weird other than shoot themselves in the foot? I assume not because approve will ensure they can only spend their own tokens.

yeah it's only "shoot themselves in the foot" scenarios:

sender can approve transfer only of his tokens to any _spender contract address. It's basically the same as sender can transfer his tokens to any contract address.

_spender contract might not comply with tokenRecipient interface i.e. doesn't have receiveApproval() function, in that case _spender still authorized to spend amount manually. So responsibility is for the sender where he's sending tokens

Shadowfiend · 2018-02-22T17:19:37Z

-  function getGrants(address _beneficiary) public constant returns (uint256[]) {
-    return grantIndices[_beneficiary];
+  function getGrants(address _beneficiaryOrCreator) public constant returns (uint256[]) {
+    return grantIndices[_beneficiaryOrCreator];


Should we be tracking these separately? In particular, should we require that you call different functions to get grants you made vs grants you received, rather than using one function for both?

yeah was my thought initially but then I was doing web UI dashboard and it seemed easier to getGrants in one call and filter by creator/beneficiary on the frontend since grant struct clearly differentiates beneficiary/creator, but maybe I'm trying to be too clever haha

Separating looks bit more code, I'm 50/50 on this let me know what you think and I'll update to something like this

grantCreatorIndices[_address]; grantBeneficiaryIndices[_address]; function getGrantsByCreator(address _address) ... function getGrantsByBeneficiary(address _address) ...

Would be interested in @mhluongo 's thoughts on this one.

Shadowfiend · 2018-02-22T17:20:42Z

+    // Cleanup.
+    delete withdrawals[_id];
+
    FinishedUnstake();


Would it make sense to carry the id with this event? Can't remember if I already asked this heh.

I'm thinking you won't get data with that id anymore, but might be useful just in case, I'll update

Shadowfiend · 2018-02-22T17:44:46Z

@@ -0,0 +1 @@
+## KEEP Network smart contracts


This should be “Keep Network”. KEEP will only ever be used to refer to the token.

ngrinkevich added 2 commits November 15, 2017 16:36

Add KEEP Token Contract

ddceb87

Tweak solidity CI setup

5cf2b75

ngrinkevich requested a review from mhluongo November 16, 2017 18:41

Add staking to KEEP token

dc3e402

mhluongo reviewed Nov 18, 2017

View reviewed changes

Minor update

c0c341a

mhluongo suggested changes Nov 20, 2017

View reviewed changes

mhluongo requested a review from Shadowfiend November 20, 2017 04:14

Split stake and vest into separate contracts

a498f7d

ngrinkevich added 6 commits November 30, 2017 16:40

Add withdrawal delay to staking contract

c1ea10a

Merge branch 'master' into contracts

a210580

Add tests for staking functionality

5a5c8c7

Add tests for vesting. Add throw proxy for testing throws.

fe8e8b6

Add more tests. Cleanup

2305d02

Add js tests for contracts

234d2d9

WIP refactor contracts with approve/transferFrom

d48a7a5

Update tests

51fb2f3

Change decimals to uint8 to match the ERC20. Set supply to 1B

e4e9506

Update zeppelin version. Add testnet config

36d45f1

Shadowfiend reviewed Jan 30, 2018

View reviewed changes

mhluongo mentioned this pull request Feb 6, 2018

Add a public key -> stake registry #29

Open

ngrinkevich added 4 commits February 15, 2018 15:13

Add testnet host

793f669

Update token staking to use approveAndCall pattern

cf4e312

Split tests to avoid 'out of gas' issue

c912175

Merge branch 'master' into contracts

8ff82cc

Shadowfiend reviewed Feb 16, 2018

View reviewed changes

ngrinkevich added 3 commits February 19, 2018 16:41

'Zero out' withdrawal record after finish unstake

ea2b79b

Add initial doxity files

38a35ad

Add beneficiary to the grants IDs mapping for easier queries

1afdcd6

Shadowfiend reviewed Feb 22, 2018

View reviewed changes

ngrinkevich added 3 commits February 22, 2018 20:56

Cleanup withdrawal indices after withdrawal is finished

a299e58

Minor updates

5cc5e75

Merge branch 'master' into contracts

36f63c0

mhluongo approved these changes Feb 28, 2018

View reviewed changes

Merge branch 'master' into contracts

b1c9a00

Shadowfiend approved these changes Feb 28, 2018

View reviewed changes

mhluongo merged commit 978d68f into master Feb 28, 2018

Shadowfiend deleted the contracts branch February 28, 2018 22:01

This was referenced May 26, 2026

feat(tbtc/signer): mirror FROST/ROAST Rust signer from tBTC monorepo #4005

Open

[DRAFT - decision-gated] feat(tbtc/signer): add TEE hardening checker stack #4007

Draft

		@@ -0,0 +1,50 @@
		pragma solidity ^0.4.18;

		import 'zeppelin-solidity/contracts/token/MintableToken.sol';


		mapping(address => uint256) stakeBalances;

		event StakeIn(address indexed from, uint256 value);

Conversation

ngrinkevich commented Nov 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhluongo commented Nov 20, 2017

Uh oh!

mhluongo commented Nov 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ngrinkevich commented Nov 24, 2017

Uh oh!

Shadowfiend commented Nov 29, 2017

Uh oh!

ngrinkevich commented Nov 29, 2017

Uh oh!

Shadowfiend commented Nov 29, 2017

Uh oh!

ngrinkevich commented Nov 29, 2017

Uh oh!

Shadowfiend commented Nov 29, 2017

Uh oh!

mhluongo commented Dec 10, 2017

Uh oh!

mhluongo commented Dec 10, 2017

Uh oh!

ngrinkevich commented Dec 10, 2017

Uh oh!

mhluongo commented Dec 10, 2017

Uh oh!

ngrinkevich commented Dec 11, 2017

Uh oh!

mhluongo commented Jan 24, 2018

Uh oh!

Shadowfiend commented Jan 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shadowfiend commented Feb 5, 2018

Uh oh!

mhluongo commented Feb 6, 2018

Uh oh!

Shadowfiend commented Feb 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

ngrinkevich commented Nov 16, 2017 •

edited

Loading

mhluongo commented Nov 20, 2017 •

edited

Loading