-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Verify Voucher locks in VoucherValidUnlocked #5609
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good assuming it doesn't break anything, lint is unhappy though
@@ -344,6 +344,7 @@ func (pm *Manager) trackInboundChannel(ctx context.Context, ch address.Address) | |||
return pm.store.TrackChannel(stateCi) | |||
} | |||
|
|||
// TODO: secret vs proof doesn't make sense, there is only one, not two |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we had both at some point in the past
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We did. But it wasn't usable so we removed it from actors.
Lint is not happy due to there being extra function in tests. In future, it might be used. |
@jennijuju can you take a look at getting this merged. I thought it was merged ages ago. |
Signed-off-by: Jakub Sztandera <[email protected]>
Signed-off-by: Jakub Sztandera <[email protected]>
Signed-off-by: Jakub Sztandera <[email protected]>
@@ -344,6 +344,7 @@ func (pm *Manager) trackInboundChannel(ctx context.Context, ch address.Address) | |||
return pm.store.TrackChannel(stateCi) | |||
} | |||
|
|||
// TODO: secret vs proof doesn't make sense, there is only one, not two |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We did. But it wasn't usable so we removed it from actors.
@Stebalien are you still requesting changes here or did we clear it up? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need a bit of followup work, but this is a strict improvement.
@Kubuxu please followup with a patch to test this. |
No description provided.