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

PoC discount on fees when tokens are staked #13

Merged
merged 9 commits into from
May 30, 2018
Merged

PoC discount on fees when tokens are staked #13

merged 9 commits into from
May 30, 2018

Conversation

johnhforrest
Copy link
Contributor

@johnhforrest johnhforrest commented May 29, 2018

Background

In the white paper we talk at length about the ability for users to stake tokens to receive reduced discounts on the protocol fees. This is a proof of concept implementation of how this will work.

In the current implementation, rather than receive a discount for the weight of a stake for a given address (this will be calculated by the time + size of stakes), it's a binary operation that disables fees.

Updates

  • Wrote a basic implementation of ERC900: Simple Staking Interface. The EIP isn't merged so the interface may change, but sufficient for the PoC.
  • Added the ability to set fees on other actions (modify, transfer)
  • Improved test case coverage when fees are enabled and when tokens are staked

Future work

  • Weight based discount
  • Proxy the staking contract so that it's upgradeable

Copy link
Member

@Anaphase Anaphase left a comment

Choose a reason for hiding this comment

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

This is gonna require some merge conflict resolution with #12 too

@@ -29,19 +23,23 @@ module.exports = async (deployer, network, accounts) => {

case 'rinkeby':
erc20TokenAddress = '0xb05e292f89c6a82f5ed1be694dc7b6444866b364'
initialFees = 10
initialFees = 10 ** 18 // 1 token
Copy link
Member

Choose a reason for hiding this comment

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

This may need to be a BigNumber, otherwise it'll loose some precision.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@@ -3,16 +3,10 @@ const CodexTitle = artifacts.require('./CodexTitle.sol')
const CodexTitleProxy = artifacts.require('./CodexTitleProxy.sol')

module.exports = async (deployer, network, accounts) => {
const proxiedCodexTitle = CodexTitle.at(CodexTitleProxy.address)
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised CodexTitleProxy.address works without first waiting for CodexTitleProxy.deployed()... how does it know the deployed address? From the previous migration steps I guess?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, from the previous migration step. We were already doing this in one other location, just decided to do it here because it's a bit cleaner

@@ -22,6 +23,10 @@ export default async function modifyMetadataHashes({
return
}

// If fees are enabled, a Transfer event is fired in addition to the Modified event
const expectedLogsLength = feesEnabled ? 2 : 1
const logIndex = feesEnabled ? 1 : 0
Copy link
Member

Choose a reason for hiding this comment

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

This should maybe be something like modifiedLogIndex instead of just logIndex.

hashedMetadata: {
name: web3.sha3('First token'),
description: web3.sha3('This is the first token'),
images: ['asdf'].map((image) => {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just make this:

images: [web3.sha3('image data')],

Since you're not using the un-hashed versions anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

hashedMetadata: {
name: web3.sha3('First token'),
description: web3.sha3('This is the first token'),
images: ['asdf'].map((image) => {
Copy link
Member

Choose a reason for hiding this comment

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

Same applies here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@johnhforrest
Copy link
Contributor Author

Will merge & rebase when addressing comments

* @title ERC900BasicStakeContainer
*/
contract ERC900BasicStakeContainer is ERC900 {
using SafeMath for uint256;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some gas optimization is possible here if we deploy the library separately and then point to it (as opposed to bundling it with both contracts)

}

require(
codexToken.transferFrom(msg.sender, feeRecipient, calculatedFee),
Copy link
Contributor

Choose a reason for hiding this comment

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

The docs for transferFrom say Usage of this method is discouraged, use safeTransferFrom whenever possible which seems to safely handle transfers to contracts. Should we use that instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already know at this point the user who sent the tokens can hold tokens, so no harm in using transfer directly here and saving a bit of gas.

@johnhforrest johnhforrest merged commit 0139df3 into master May 30, 2018
@johnhforrest johnhforrest deleted the erc900 branch June 5, 2018 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants