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

Feature/split contract #58

Merged
merged 37 commits into from Dec 5, 2018

Conversation

Projects
None yet
4 participants
@anxolin
Copy link
Contributor

anxolin commented Nov 22, 2018

Hi @Velenir and @josojo !

Could you help review this PR.
@josojo I include you because I changed things in the smart contract, I would appreciate if you review this parts.

There's several things I do here:

  1. I added a src/migration/migrations-truffle-1.5 so other projects that are already in truffle-1.5 can deploy the dutchx contracts. I keep also the src/migration/migrations until we migrate the whole project

  2. I took some logic out of the main contract, and created some smaller contracts with them:

image

So the main contract inherit this other contracts:
image

In the future I plan to break it more. But this is the first round. I did it because I wanted in another project to use the contract TokenWhitelist.sol that I just created.

  1. @josojo Please, note that I think there's only a small change. I duplicated the WAITING_PERIOD_CHANGE_MASTERCOPY_OR_ORACLE const making 2 different constants.

  2. I migrated in a similar fashion gno-token, util-contracts and owl-token projects. I didn't create PRs for them cause I think is to much overhead.

Let me know your thoughts please.

Thanks!

@anxolin anxolin requested a review from Velenir Nov 22, 2018

Show resolved Hide resolved contracts/base/AuctioneerManaged.sol Outdated
Show resolved Hide resolved contracts/base/AuctioneerManaged.sol Outdated
@josojo
Copy link
Contributor

josojo left a comment

Hi,

like the new structure. It also enhances readability etc.

-> could you explain to me the inheritance of constructor? I mentioned it in the comments
-> could you update all the tests?

-> I will review later the migration scripts

@anxolin

This comment has been minimized.

Copy link
Contributor Author

anxolin commented Nov 23, 2018

The migration scripts is OK, if you want to read them is fine, but I'm not too worried about them, because for this project we still use truffle 4.

I just provided an alternative truffle 5 migration files for others to use (especially for local development). I use it in the dx-daostack project for example.

I'm more interested in the smart contract changes, cuase I don't want to break the audit :)

I'll review now the tests.

THANKS FOR YOUR COMMENTS!

@anxolin

This comment has been minimized.

Copy link
Contributor Author

anxolin commented Nov 23, 2018

The tests pass, but it takes so much time that sometime Travis fails...
I just retried and it worked:

image

@@ -1,44 +1,49 @@
pragma solidity ^0.4.21;
pragma solidity ^0.4.24;


contract DSAuthority {

This comment has been minimized.

@josojo

josojo Dec 4, 2018

Contributor

Modifying external contracts( contracts from Maker) is not a good practice. Have you double checked that they don't have any new version? Probably, it's hard to adapt to their new versions, right?
Especially with their migration to multi collateral, they probably have developed new price feed mechanism.

This comment has been minimized.

@anxolin

anxolin Dec 4, 2018

Author Contributor

Yes I agree.

The thing is we use this just for the test in local ganache, and we just want to remove the warnings, cause we have so many of them that we cannot pay attention to ours anymore.

I think we should depend directly from their projects, but we are out of capacity for this, given that is just for the tests and we are not altering any logic. I take note so we might do it in the future, also happy if u PR us the proposal :)

Show resolved Hide resolved contracts/base/EthOracle.sol Outdated
@josojo

josojo approved these changes Dec 4, 2018

Copy link
Contributor

josojo left a comment

I especially like the fine-tuned migration scripts. Good job

@anxolin anxolin merged commit a6e80b8 into develop Dec 5, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@anxolin anxolin deleted the feature/split-contract branch Dec 5, 2018

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