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

Refactor #78

Merged
merged 14 commits into from Dec 3, 2018
Merged

Refactor #78

merged 14 commits into from Dec 3, 2018

Conversation

eezcjkr
Copy link
Contributor

@eezcjkr eezcjkr commented Nov 27, 2018

No description provided.

import "./TxLib.sol";
import "./PriorityQueue.sol";

contract ExitHandler is DepositHandler {
Copy link
Member

Choose a reason for hiding this comment

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

This looks strange from OOP point of view

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How so?

Copy link
Member

Choose a reason for hiding this comment

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

OOP would be something like:
Vault <- DepositableVault <- ExitableVault
?

Copy link
Member

Choose a reason for hiding this comment

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

Since we have multiple inheritance it can be Vault ← DepositableVault & ExitableVault. But it's really a nitpicking :-) I know too few about solidity to have a voice 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.

Makes sense. The most reasonable relationship would be to have a composition I guess, that is a Vault has a DepositHandler and it has an ExitHandler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But we will need a separate exit procedure for unincluded deposits so we will need a version of ExitHandler that is aware of the deposits.

Copy link
Member

Choose a reason for hiding this comment

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

As i said, you can ignore me here :-)

.gitignore Outdated Show resolved Hide resolved
@johannbarbie johannbarbie merged commit 6c6997c into master Dec 3, 2018
@troggy troggy deleted the refactor branch December 4, 2018 09:34
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.

None yet

4 participants