[WIP] brian/rebalancing_set_basic #167
Conversation
Pull Request Test Coverage Report for Build 1365
💛 - Coveralls |
| @@ -0,0 +1,389 @@ | |||
| /* | |||
There was a problem hiding this comment.
Maybe we can move the SetToken, SetTokenFactory, RebalancingSet, and RebalancingSetFactory into their own directory
There was a problem hiding this comment.
Sure...I'll leave the organization to @asoong :)
| import { ISetFactory } from "./interfaces/ISetFactory.sol"; | ||
|
|
||
| /** | ||
| * @title SetToken |
There was a problem hiding this comment.
RebalancingToken is the verbage we settled on
| /* ============ Constructor ============ */ | ||
|
|
||
| /** | ||
| * Constructor function for Rebalancing Set Token |
There was a problem hiding this comment.
Perhaps we note that this initial Rebalancing Set uses a manager vs. other oracle system
There was a problem hiding this comment.
In my mind this can use either. It's just an address, all someone needs to do is prop up a smart contract that acts as the oracle and set that as the manager.
| */ | ||
|
|
||
| constructor( | ||
| address _factory, |
There was a problem hiding this comment.
Do we want this Rebalancing Set to conform to a normal Set? So that it includes components, units, naturalUnit, etc.?
There was a problem hiding this comment.
Ah I see from below that share Ratio is just the internal naming
| DetailedERC20(_name, _symbol, 18) | ||
| { | ||
| // Require day long proposal period | ||
| require(_proposalPeriod > 86400); |
There was a problem hiding this comment.
I think ethereum has a global variable / unit called day
https://solidity.readthedocs.io/en/v0.4.24/units-and-global-variables.html?highlight=day
There was a problem hiding this comment.
Cool yeah still not sure what I wanna set that at, but yeah that's a good thing to use.
| external | ||
| { | ||
| // Must be in "Proposal" state before going into "Rebalance" state | ||
| require(rebalanceState == State.Proposal); |
There was a problem hiding this comment.
We could probably abstract the state transition and timing checks into a private function
There was a problem hiding this comment.
Can you explain what you mean? If it's what I think you mean I suppose that would look cleaner but not sure it'd be more efficient.
There was a problem hiding this comment.
It's just an optimization (can be done later), but we can have a state transition function so we dedupe the logic.
| require(block.timestamp >= lastRebalanceTimestamp.add(rebalanceInterval)); | ||
|
|
||
| // Set auction parameters | ||
| rebalancingSet = _rebalancingSet; |
There was a problem hiding this comment.
We should have a check to ensure that the address proposed is indeed a Set, perhaps querying Core
There was a problem hiding this comment.
Yeah I'd be ok with that.
| function getComponents() | ||
| external | ||
| view | ||
| returns(address[1]) |
There was a problem hiding this comment.
Is this more efficient? I think the normal getComponents interface is just address[]. Wonder if there's any issues with address[1]
There was a problem hiding this comment.
It wouldn't let me return a dynamic address for whatever reason. Perhaps because what's being returned isn't dynamic in nature?
There was a problem hiding this comment.
This might be an issue when we go to issue (lol)...I'll have to consider ways to be able to return a dynamic address array
contracts/core/SetToken.sol
Outdated
| public | ||
| returns (bool) | ||
| { | ||
| // Confirm address is not null |
There was a problem hiding this comment.
Any reason we remove this
There was a problem hiding this comment.
It's duped...that logic already exists in the StandardToken contract which we call through super.
bweick
force-pushed
the
brian/rebalancing_set_basic
branch
from
b5dcf16 to
516040d
Compare
| external | ||
| { | ||
| // Must be in "Proposal" state before going into "Rebalance" state | ||
| require(rebalanceState == State.Proposal); |
There was a problem hiding this comment.
It's just an optimization (can be done later), but we can have a state transition function so we dedupe the logic.
| const RebalancingToken = artifacts.require('RebalancingToken'); | ||
|
|
||
| contract('RebalancingSet', accounts => { | ||
| contract('RebalancingToken', accounts => { |
There was a problem hiding this comment.
Should we normalize to rebalancing set or rebalancing token?
3 participants
First run implementation of rebalancing set. WIP. Testing not complete b/c need to find way to more gracefully move through states and need future functionality for full test.