Extract handover functions to the new contract - #431
Conversation
2336e35 to
dfc2669
Compare
dfc2669 to
136a375
Compare
vzotova
changed the title
manumonti
left a comment
manumonti
left a comment
There was a problem hiding this comment.
LGTM! 👌, but I have some questions
|
|
||
| // Storage area for sentinel values | ||
| uint256[15] internal __preSentinelGap; | ||
| uint256[14] internal __preSentinelGap; |
There was a problem hiding this comment.
What is the reason to modifying the size of this from 15 to 14? Just curious.
There was a problem hiding this comment.
because another slot from the gap was consumed by adding handoverCoordinator
| mapping(bytes32 handoverKey => Handover handover) public handovers; | ||
| // Note: Adjust the __preSentinelGap size if more contract variables are added | ||
|
|
||
| uint256[20] internal __gap; |
There was a problem hiding this comment.
Not understanding what is this variable.
There was a problem hiding this comment.
in case we inherit contract and then upgrade it we want to have ability to add new variables to that contract, so we reduce gap and add new variable without any changes in child contract.
|
|
||
| ITACoChildApplication public immutable application; | ||
| Coordinator public immutable coordinator; | ||
| uint32 public immutable handoverTimeout; |
There was a problem hiding this comment.
I know this is not the purpouse of this PR, but I'm curious: why making handoverTimeout immutable and not something that we can modify?
There was a problem hiding this comment.
cheaper on gas, basically it's constant that can be changed by upgrade
| if (t0 == 0) { | ||
| return HandoverState.NON_INITIATED; | ||
| } else if (block.timestamp > deadline) { | ||
| // Handover failed due to timeout |
There was a problem hiding this comment.
Probably I'm misunderstanding how handovers work, but if we reached the timeout, it doesn't necessary means that the handover failed, right? The handover could be succesful AND we have reached the timeout.
There was a problem hiding this comment.
if it's outside timeout it means that blinded share and transcript were not provided in time which means something wrong with one of the nodes
There was a problem hiding this comment.
Ah, right, right. When handover is finished requestTimestamp is set to 0. I didn't realize that.
| handover.requestTimestamp = 0; | ||
| handover.incomingProvider = address(0); | ||
| delete handover.blindedShare; | ||
| delete handover.transcript; |
There was a problem hiding this comment.
Not the scope of this PR, but we should consider deleting the transcripts of all participants since these are no longer valid (can't recreate an transcript aggregation after a handover). See #427
derekpierre
left a comment
derekpierre
left a comment
There was a problem hiding this comment.
This change would imply changes to nucypher to use the HandoverCoordinator instead of Coordinator contract? i.e. we will need a HandoverCoordinatorAgent and updated calls made to that agent for handovers?
There was a problem hiding this comment.
This constant is no longer needed in this file.
| @@ -0,0 +1,22 @@ | |||
| #!/usr/bin/python3 | |||
There was a problem hiding this comment.
Does this script work? I think you might need a separate script and yml file for just deploying/upgrading the handover coordinator because the values needed for the deployment of HandoverCoordinator are obtained from other prior deployments and NOT from the registry?
There was a problem hiding this comment.
Also the handover timeout in child.yml (initial deployment) is old releative to upgrade-coordinator (latest deployment update).
There was a problem hiding this comment.
added new scripts and configs
3 participants
Type of PR:
Required reviews:
What this does:
Issues fixed/closed:
Why it's needed:
Notes for reviewers: