feat: handle rail termination callback from payments contract #87
Conversation
ZenGround0
left a comment
ZenGround0
left a comment
There was a problem hiding this comment.
This is good. We'll want to integrate this in with dataset removal when we get around to #69.
| RailTerminationStatus memory termStatus = railTerminationStatus[info.railId]; | ||
| require( | ||
| !termStatus.isTerminated || block.number <= termStatus.endEpoch, | ||
| "Operation rejected: rail terminated and beyond end epoch" |
There was a problem hiding this comment.
Since these errors really mean -- "this entire dataset is unrecoverable" it should probably be a little more descriptive in this direction.
For example
"Operation rejected: rail terminated and finalized -- proof set must be removed to make progress"
There was a problem hiding this comment.
I think we can remove the prefix Operation rejected, since user will already see in the transaction data that it has been required
| require(dataSetId != 0, "Rail not associated with any data set"); | ||
|
|
||
| // Update termination status | ||
| railTerminationStatus[railId] = RailTerminationStatus({ |
There was a problem hiding this comment.
nit: you could make this is a simple mapping[uint256] => uint256 just storing the end epoch and letting a 0 value stand in for "not terminated"
github-project-automation
bot
moved this from 🔎 Awaiting review
to ✔️ Approved by reviewer
in FS
| RailTerminationStatus memory termStatus = railTerminationStatus[info.railId]; | ||
| require( | ||
| !termStatus.isTerminated || block.number <= termStatus.endEpoch, | ||
| "Operation rejected: rail terminated and beyond end epoch" | ||
| ); |
There was a problem hiding this comment.
can we refactor this into a method like requireActivePaymentRail(railId) or requireUnterminatedPaymentRail(railId)? It is being used without differences in 3 places
There was a problem hiding this comment.
I can add termination logic for the FilCDN rails when we have decided on this matter
| RailTerminationStatus memory termStatus = railTerminationStatus[info.railId]; | ||
| require( | ||
| !termStatus.isTerminated || block.number <= termStatus.endEpoch, | ||
| "Operation rejected: rail terminated and beyond end epoch" |
There was a problem hiding this comment.
I think we can remove the prefix Operation rejected, since user will already see in the transaction data that it has been required
| * @dev Implements the IValidator interface function | ||
| * @param railId ID of the payment rail being terminated | ||
| * @param terminator Address that initiated the termination | ||
| * @param endEpoch The final epoch up to which the rail can be settled |
There was a problem hiding this comment.
I might not understand settling fully, but does this mean funds will be locked inside the rail if I didn't settle them before endEpoch?
| */ | ||
| function railTerminated(uint256 railId, address terminator, uint256 endEpoch) external override { | ||
| // Only payments contract can call this | ||
| require(msg.sender == paymentsContractAddress, "Only payments contract can terminate rails"); |
There was a problem hiding this comment.
since only the one payments contract can do this, shall we drop the terminator argument?
There was a problem hiding this comment.
@juliangruber The terminator is the end user who terminated (payer, payee or operator ?). I'll improve the name.
There was a problem hiding this comment.
If that is the case then the error message in the require() is confusing
github-project-automation
bot
moved this from ✔️ Approved by reviewer
to ⌨️ In Progress
in FS
|
Closing this in favour of "#93". |
Now that we have a
railTerminatedcall back from the payments contract:Once a rail is terminated:
SP can not add new roots to the proofset as we can't increase the rate of a terminated rail
After the rail end epoch has elapsed (i.e. the rail "expires"):