Bug add days #22
Bug add days #22
Conversation
| @@ -0,0 +1,3188 @@ | |||
| // SPDX-License-Identifier: MIT | |||
|
|
|||
| pragma solidity ^0.6.0; | |||
| address oracle, | ||
| bool allowed, | ||
| bool exists); | ||
| event RequestActivityDistanceFulfilled(bytes32 indexed requestId, uint256 indexed distance); |
There was a problem hiding this comment.
Is this the event our app is waiting on? If so, should we include the committer's address as well?
contracts/SinglePlayerCommit.sol
Outdated
| @@ -163,84 +149,72 @@ contract SinglePlayerCommit is ChainlinkClient, Ownable { | |||
| /// @notice Create commitment, store on-chain and emit event | |||
| /// @param _activityKey Keccak256 hashed, encoded name of activity | |||
| /// @param _goalValue Distance of activity as goal | |||
| /// @param _startTime Starttime of commitment, also used for endTime | |||
| /// @param _daysToStart Starttime of commitment, also used for endTime | |||
There was a problem hiding this comment.
I think we should generalize this to _timeToStart for greater precision. For example, we should be able to handle scenarios when a user wants to start in a few hours, or some number of hours and minutes.
Also, lets update the comment to reflect the new param.
contracts/SinglePlayerCommit.sol
Outdated
| userId: _userId, | ||
| exists: true | ||
| }); | ||
| uint256 startTime = _daysToStart > 0 ? addDays(_daysToStart, block.timestamp): block.timestamp; |
There was a problem hiding this comment.
Similarly here, I think we may need to user something other than the addDays function, e.g. if the offset is in hours or fractions of days.
contracts/SinglePlayerCommit.sol
Outdated
| _startTime, | ||
| endTime, | ||
| _stake); | ||
| emit NewCommitment(msg.sender, activities[_activityKey].name, _goalValue, _daysToStart, endTime, _stake); |
There was a problem hiding this comment.
let's include startTime instead of _daysToStart in the event
contracts/SinglePlayerCommit.sol
Outdated
|
|
||
| return true; | ||
| } | ||
|
|
||
| /// @notice Wrapper function to deposit <token> and create commitment in one call | ||
| /// @param _activityKey Keccak256 hashed, encoded name of activity | ||
| /// @param _goalValue Distance of activity as goal | ||
| /// @param _startTime Starttime of commitment, also used for endTime | ||
| /// @param _daysToStart Starttime of commitment, also used for endTime |
There was a problem hiding this comment.
same change here to _timeToStart
contracts/SinglePlayerCommit.sol
Outdated
| _userId | ||
| ), "SPC::depositAndCommit - commitment creation failed"); | ||
| require( | ||
| makeCommitment(_activityKey, _goalValue, _daysToStart, _amountOfDays, _stake, _userId), |
closes #6
on #6:
Discord: "If I understand the case correctly, we could check on the timestamp of the previous block (as a threshold for 'the past'), but that is not something that could be done without querying the chain. My proposal would be to drop this and fix it in the front-end. So a CommitPool application specific requirement instead of a contract requirement. "