Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
multi: log additional information for local force closures #8072
base: master
Are you sure you want to change the base?
multi: log additional information for local force closures #8072
Changes from all commits
844681d
c60ea99
b22d72d
e67dabd
6824a82
6fc6915
65e7d6a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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 don't think this is the right strategy to go for here. Using continuations to implement this is "too powerful" and we probably just want to explicitly enumerate the cases.
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.
Okay could you please explain why this is so? I got that it is "too powerful" but that was not backed up by anything really.
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.
It is too powerful because you are using a full continuation here. Someone could supply a function that does anything and it is delegating the process of calling the "log" functions to the argument. If you look at ChainArbitrator.ForceCloseContract it is simply calling out to the supplied
opt
do do the log action. However, all of what you need to do with this is finitely enumerable and we have no reason to believe it ever won't be. So you should just take a regular data parameter and then call the appropriate log action depending on that value.Continuation passing has its uses but I don't think this is a good instance of it.
That said, I wouldn't waste time taking a look at these smaller comments before zooming out and reworking the approach. See my other comment. I think @ziggie1984 has the right idea of what to do here.
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 also don't think it's the right approach - 1) it's not really an option, but an extra step in
ForceCloseContract
where we save the info 2) this can be expanded indefinitely, instead we should limit the function scope here. I think you could define aForceCloseReason
struct here and let other callers use it inForceCloseContract
. Just a rough thought, need to think about it moreThere 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 don't quite get this @yyforyongyu. Care explaining more about this?
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.
This should accept a
LinkFailureError
as opposed to astring
. We should move the type to a separate package if necessary to accomplish this.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.
Could you please explain why this should be stored as a
LinkFailureError
as opposed to string? Note that why this errorMsg is converted to channeldb'sLocalForceCloseInitiator
type and stored in this function. The string contains the reason for the Link failure which is the necessary information that we need any way.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.
It's not that it needs to be stored as a LinkFailureError. But this function should accept a LinkFailureError for type safety reasons. It enforces stricter discipline at potential callsites