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.
contracts/payment/RefundEscrow.sol
Outdated
| * @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.
There was a problem hiding this comment.
that helped, thanks!
contracts/ownership/Claimable.sol
Outdated
| } | ||
|
|
||
| /** | ||
| * @dev Allows the pendingOwner address to finalize the transfer. | ||
| */ | ||
| 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
| @@ -40,5 +40,6 @@ contract Claimable is Ownable { | |||
| */ | |||
| 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. |
This is part of #1174
Requires #1254.
npm run lint:all:fix).