Improve encapsulation on lifecycle, ownership and payments #1269
Conversation
come-maiz
added
kind:improvement
contracts
frangio
force-pushed
the
refactor/1174/private-state-vars
branch
from
bcc442e to
fe431bf
Compare
|
Rebased on |
contracts/ownership/Ownable.sol
Outdated
| /** | ||
| * @return the address of the owner. | ||
| */ | ||
| function getOwner() public view returns(address) { |
There was a problem hiding this comment.
I'm not fond of starting this convention of getters prefixed with get. I feel like it goes against the rest of the ecosystem.
There was a problem hiding this comment.
This is what we discussed about having verbs on function names. I won't like it, but I can remove the gets and try to convince the ecosystem to use them before 3.0.
There was a problem hiding this comment.
Yes, I'd prefer to remove the get.
| * @return the current state of the escrow. | ||
| */ | ||
| function getState() public view returns(State) { | ||
| return state_; |
There was a problem hiding this comment.
any idea how to test this?
When I do something like:
(await this.escrow.getState()).should.be.equal(1);
I get:
AssertionError: expected { Object (s, e, ...) } to equal 1
I might be doing something dumb here.
There was a problem hiding this comment.
I think the error just means that they're not the same thing. Try using should.be.bignumber.equal to get a nicer error message.
contracts/ownership/Claimable.sol
Outdated
| function claimOwnership() public onlyPendingOwner { | ||
| emit OwnershipTransferred(owner, pendingOwner); | ||
| owner = pendingOwner; | ||
| pendingOwner = address(0); |
frangio
dismissed
their stale review
Should not be merged until previous comment is fixed or #1274 is merged.
|
@ElOpio Can you fix the merge conflicts please? |
contracts/ownership/Claimable.sol
Outdated
| */ | ||
| function claimOwnership() public onlyPendingOwner { | ||
| _transferOwnership(pendingOwner_); | ||
| pendingOwner = address(0); |
There was a problem hiding this comment.
Missing underscore suffix! 😣
There was a problem hiding this comment.
goddammit! I should have breakfast first :D
|
@frangio resolved |
|
There's something weird with the coverage report. The lines it report uncovered have specific tests added on this PR, and that lead to full coverage on previous commits. Maybe, I broke something on one merge, or maybe it's coveralls being nuts. |
frangio
left a comment
frangio
left a comment
There was a problem hiding this comment.
I'm not sure why coverage decreased either. It could be related to only testing those functions with actual members of the mapping... But this shouldn't be a problem. Let's merge anyway. Thanks!
2 participants
This is part of #1174
Requires #1254.
npm run lint:all:fix).