Skip to content

Conversation

@cwhinfrey
Copy link
Contributor

No description provided.

@cwhinfrey cwhinfrey requested a review from mikec February 19, 2018 16:24
@@ -0,0 +1,9 @@
pragma solidity ^0.4.18;
Copy link
Contributor

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';
Copy link
Contributor

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 {
Copy link
Contributor

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 {
Copy link
Contributor

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.

@mikec
Copy link
Contributor

mikec commented Feb 20, 2018

@cwhinfrey just a few comments. some organizational stuff we should think about changing, but we can play with it after it's merged

@mikec
Copy link
Contributor

mikec commented Feb 22, 2018

forgot to 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants