-
Notifications
You must be signed in to change notification settings - Fork 10
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
Conversation
There was a problem hiding this 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
migrations/6_initialize_721token.js
Outdated
@@ -29,19 +23,23 @@ module.exports = async (deployer, network, accounts) => { | |||
|
|||
case 'rinkeby': | |||
erc20TokenAddress = '0xb05e292f89c6a82f5ed1be694dc7b6444866b364' | |||
initialFees = 10 | |||
initialFees = 10 ** 18 // 1 token |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
test/helpers/modifyMetadataHashes.js
Outdated
@@ -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 |
There was a problem hiding this comment.
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
.
test/token/CodexTitle.test.js
Outdated
hashedMetadata: { | ||
name: web3.sha3('First token'), | ||
description: web3.sha3('This is the first token'), | ||
images: ['asdf'].map((image) => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
test/token/CodexTitleProxy.test.js
Outdated
hashedMetadata: { | ||
name: web3.sha3('First token'), | ||
description: web3.sha3('This is the first token'), | ||
images: ['asdf'].map((image) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same applies here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Will merge & rebase when addressing comments |
* @title ERC900BasicStakeContainer | ||
*/ | ||
contract ERC900BasicStakeContainer is ERC900 { | ||
using SafeMath for uint256; |
There was a problem hiding this comment.
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), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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
Future work