Skip to content
This repository was archived by the owner on Jan 18, 2023. It is now read-only.

Conversation

felix2feng
Copy link
Contributor

@felix2feng felix2feng commented Aug 16, 2018

  • Authorized addresses of the vault, SetTokenFactory, TransferProxy, and TakerWalletWrapper can only be set for a month.
  • Unit testing includes use of increaseTimeAsync and snapshot reversions to handle time-based tests

@coveralls
Copy link

coveralls commented Aug 16, 2018

Pull Request Test Coverage Report for Build 1375

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 1332: 0.0%
Covered Lines: 345
Relevant Lines: 345

💛 - Coveralls

/* ============ Constructor ============ */

constructor()
Authorizable(4 weeks)
Copy link
Contributor

Choose a reason for hiding this comment

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

How does it calculate 4 weeks? Is it automatically 28 days?

Copy link
Contributor

Choose a reason for hiding this comment

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

I dont think SetTokenFactory needs Authorizable, going to change to just be Core's address

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be more explicit to note do the number of seconds vs. using a higher level construct like weeks.

OK - will remove from SetTokenFactory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@asoong I wonder if it makes sense to pass the Core address in the constructor. Do we imagine needing to change Core's address?

@felix2feng felix2feng merged commit 73cf389 into master Aug 16, 2018
@felix2feng felix2feng deleted the felix/add-time-bound-authorizations branch August 16, 2018 17:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants