Fallback for process Redemption and Staking - #60
Conversation
1dac0e4 to
287ed48
Compare
There was a problem hiding this comment.
The description of the PR indicates tests, but testing appears to be incomplete/uneven. Both OpenSTValue and OpenSTUtility update processStaking and processRedeeming tests to not use the registrar, but OpenSTValue has additional testing, of registrar's ability to process staking, that is not replicated in OpenSTUtility for redeeming; and these changes are not tested for the Registrar contract.
| address public registrar; | ||
| /// registered branded tokens | ||
| mapping(bytes32 /* uuid */ => RegisteredToken) public registeredTokens; | ||
| mapping(bytes32 /* uuid */ => RegisteredToken) registeredTokens; |
There was a problem hiding this comment.
Why drop the public modifier?
There was a problem hiding this comment.
because it s a struct, and so a public getter cannot be implemented by the compiler (it cannot return a struct, correct?); in exchange we have registeredTokenProperties
There was a problem hiding this comment.
But then I think that should mean we are seeing compiler errors on develop. Also, when I called the public function just now in what's on the develop branch, it returns two (sensible looking) addresses.
There was a problem hiding this comment.
so then structs can be auto-decomposed by the compiler; there does not need to be both though. Then we should remove likewise functions on OpenSTValue.sol as well; I'll open an issue for it instead, that updates the integration tests that use these getters.
| Redemption storage redemption = redemptions[_redemptionIntentHash]; | ||
| require(redemption.redeemer == msg.sender); | ||
|
|
||
| // note: as processRedemption incurs a cost for the staker, we provide a fallback |
There was a problem hiding this comment.
Should this refer to "redeemer"? I see arguments for both ways, so mostly just asking to confirm this is intentional and not simply a copy/paste oversight.
There was a problem hiding this comment.
copy-paste oversight; thx
| Redemption storage redemption = redemptions[_redemptionIntentHash]; | ||
| require(redemption.redeemer == msg.sender); | ||
|
|
||
| // note: as processRedemption incurs a cost for the staker, we provide a fallback |
There was a problem hiding this comment.
I think a fair reading of this comment implies that the issue is that there is a cost to the redeemer and the fix is to provide a fallback to mitigate that cost in someway (e.g., if the redeemer were unable to call processRedemption for some reason and problems issuing from that deficiency).
This comment should be reworded to clarify that the issue to address is that the redeemer could avoid certain costs, but that such avoidance is counter to the protocol; and that the fallback is the fix to ensure adherence to the protocol. Same comment for OpenSTValue.
There was a problem hiding this comment.
extended explanation to make this point
| uint256 expirationHeight); | ||
|
|
||
| function processRedeeming( | ||
| bytes23 _redemptionIntentHash) |
| // address of OpenSTUtility registry: | ||
| OpenSTUtilityInterface _registry, | ||
| // OpenSTUtility function: | ||
| bytes23 _redemptionIntentHash) |
|
|
||
| it('fails to process when msg.sender != staker', async () => { | ||
| await Utils.expectThrow(openSTValue.processStaking(stakingIntentHash, { from: registrar })); | ||
| // registrar can additionally as a fallback process staking in v0.9 |
There was a problem hiding this comment.
The it of this test should be updated to reflect what the comment implies—this tests that a non-staker/non-registrar cannot successfully call the function.
There was a problem hiding this comment.
changed to " fails to process when msg.sender is not staker or registrar"
| }) | ||
| }) | ||
|
|
||
| describe('ProcessStaking with fallback', async () => { |
There was a problem hiding this comment.
To be consistent, the "test stories" at the top should be updated to reflect this addition.
There was a problem hiding this comment.
updated in OpenSTValue.js and OpenSTUtility.js
|
|
||
| it('fails to process if msg.sender != redeemer', async () => { | ||
| await Utils.expectThrow(openSTUtility.processRedeeming(redemptionIntentHash, { from: accounts[1] })); | ||
| await Utils.expectThrow(openSTUtility.processRedeeming(redemptionIntentHash, { from: accounts[5] })); |
There was a problem hiding this comment.
The it of this test should be updated to reflect what the comment implies—this tests that a non-redeemer/non-registrar cannot successfully call the function.
There was a problem hiding this comment.
changed to "fails to process if msg.sender is not redeemer or registrar"
008357a to
a1ea331
Compare
|
2 participants
contracts, test: introduce fallback for process staking and redemption. fixes #26
this was forgotten to be merged into develop