-
Notifications
You must be signed in to change notification settings - Fork 0
Add delegated storage contracts #3
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
base: master
Are you sure you want to change the base?
Conversation
| @@ -0,0 +1,9 @@ | |||
| pragma solidity ^0.4.18; | |||
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 made a change right before you put this PR up that removes the struct/library thing from StorageConsumer and StorageStateful. I think this change might be adding it back in?
If you rebase the latest from master, you won't need to add this BasicStorage contract since StorageConsumer is now simplified to only have a _storage address like this one has.
| import '../DelegatedStorage/DelegatedStorage.sol'; | ||
| import '../DelegatedStorage/KeyManagerInterface.sol'; | ||
| import '../Proxy/DelegateManagerInterface.sol'; | ||
| import 'zeppelin-solidity/contracts/ownership/Ownable.sol'; |
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.
nitpick alert ... external imports should be declared before locals
| import './BaseProxy.sol'; | ||
| import './DelegateManagerInterface.sol'; | ||
|
|
||
| contract DelegateManagable is BaseProxy { |
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.
thinking about having everything that extends Proxy be named with "Proxy" .. could call this ManagedProxy or something like that
| import '../Proxy/DelegateManagable.sol'; | ||
| import './KeyManagerInterface.sol'; | ||
|
|
||
| contract DelegatedStorage is BaseStorage, DelegateManagable { |
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.
thinking we should move everything that extends BaseStorage into the /Storage dir.
|
@cwhinfrey just a few comments. some organizational stuff we should think about changing, but we can play with it after it's merged |
|
forgot to 👍 |
No description provided.