Bump keep-ecdsa to 0.10.0-rc and fix seize bonds problem #532
Conversation
We updated ECDSAKeepStub to more closely reflect IBondedECDSAKeep interface. - removed `distributeETHToMembers` function which was removed from the interface - updated order of functions in stub to be same as in the interface
We removed closeKeep call happening before seizeSignerBonds as keep requires the keep to be active to let seizeSignerBonds. Keep will be automatically closed by itself just by calling seizeSignerBonds function.
Implemented some of ECDSAKeepStub functions from IBondedECDSAKeep interface for testing.
| function distributeETHReward() external payable { | ||
| uint256 dividend = msg.value.div(memberCount); | ||
|
|
||
| require(dividend > 0, "Dividend value must be non-zero"); |
There was a problem hiding this comment.
Hmmm… I'm 50-50 on this change. At this point this ceases to be a stub. Plus we don't end up correctly testing the interface; instead, we start integration testing the stub :/
The current situation is imperfect, but I think we should stick with what we have and work on system and integration tests out of band to catch the broader issue.
Thoughts?
There was a problem hiding this comment.
I'm not a huge fan of this solution either. I wanted to have anything to catch possible integration problems sooner than later. This is definiately a case for integration tests.
There was a problem hiding this comment.
Yeah, I think I'm interested in just leaving this. We would fail to catch the next integration problem unless the change made in keep-ecdsa was made to this stub as well, which is roughly equivalent to having someone on keep-ecdsa also reviewing tbtc ecdsa-related changes, which is what we should have done in the first place when this originally happened (and which I bypassed).
I think I'd rather leave the stub as-is and pull in just the fix, tbh :/
Also I imagine since the diff didn't include it that this was clear, but we shouldn't bump the keep-ecdsa dependency to -rc in a PR to master :)
There was a problem hiding this comment.
Also I imagine since the diff didn't include it that this was clear, but we shouldn't bump the keep-ecdsa dependency to -rc in a PR to master :)
We didn't release the 0.10.0 version, but we probably should - CC @pdyraga.
There was a problem hiding this comment.
There was a problem hiding this comment.
We did release 0.10.0-rc but I believe @Shadowfiend expected 0.10.0 to be used in tbtc dependencies. Am I correct?
There was a problem hiding this comment.
We should reference 0.10.0-pre, as here: https://github.com/keep-network/tbtc.js/blob/master/package.json#L26
However, I just realized that until I finish all my packaging stuff we may not be publishing -pre versions, so… Whatever works, works.
This reverts commit fdb896a.
We set default value for success in ECDSAKeepStub to be true and added reset value used in before tests blocks to set it back to default value. We removed redundant `setSussess(true)` calls, we no longer need them as we reset it.
|
Ah ah ah, you didn't update your pre-commit 😬 Looks like you need to run |
To be honest, I did. I was just scared of the number of changes from prettier and disabled it for this PR, not to cause any confusions and handle it in dedicated formatting update PR haha
I'll do that! |
|
Actually, it wasn't prettier that changed so much formatting, it was something wrong with my VSCode settings. Updated the code, I'm sorry for the inconvenience 😉 |
|
No worries! Let's rock and roll 😎 |
This PR contains a couple of improvements to integration with keep-ecdsa:
@keep-network/keep-ecdsato0.10.0-rc.Closes: #401