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
Refactor ChanUpgradeInit to use new upgrade type #3456
Refactor ChanUpgradeInit to use new upgrade type #3456
Changes from 21 commits
995c1f1
f8f6ad4
808ec70
8dd0c44
64878ae
51bb7a7
f203df9
837e3c9
5f9ad67
7bdf33c
13812eb
2f7d0fc
28779f5
08af922
458b171
93db50e
b83849a
f29e2f8
09ddf84
8f36c16
0b36176
dfd9be3
f7eb3dd
c631e06
b8b5ca4
0e8092b
06b1271
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Would be good to add a test for this event, so that if in the future new upgrade fields are added, then the test should fail, so that we don't forget to add the field here. Alternatively, instead of adding each upgrade field with its own key, would it work to have one key for all the upgrade fields and the value is a JSON-encoded string of all of them?
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.
Are you happy if we do this in a followup, I think a few changes would need to happen to the tests (as we are testing the channel keeper function but events are emitted at the message server layer)
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.
In a separate pr, how would folks feel about making this function private and placed in
upgrade.go
?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 think I made the exported as it was used in another package, if it's possible though I'm in favour of making it unexported.
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.
What package? I feel like validation of the upgrade fields against the current fields should only happen in the init and try handlers?
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 looks like 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.
The only thing we have to be careful with about passing in the channel is to ensure it is obtained and not modified in state before calling this function
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 could do an additional read of the channel directly in the function, I'm not sure it's necessary though, WDYT?
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 probably safer to do an additional read but I'm also happy to leave as is for now. @colin-axner and I discussed yesterday briefly about refactoring grpc handlers into their associated submodules rather than having the entry point at the core layer. This would mean we could reduce the exported APIs and prevent them from being misused. If this were a private method then it would be much safer passing
currentChannel
.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 like the idea of doing the additional read for safety and then optimizing later if there's a clean way to do so, but I don't have a strong preference here. Will go with your gut!
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.
should we add a comment for why we do the minus 1?
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 can add a comment if you think it will help, I think the combination of the names
GetNextSequenceSend
andLatestSequenceSend
make it pretty clear, but I can add a comment to make it extra explicit if you think it makes it more clear.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 could also consider adding a new keeper method to sweep it under the rug a little. E.g.
k.GetLastSequenceSend(ctx, portID, channelID)
which would read the same key/value asGetNextSequenceSend
but handle the off by one.I think if we do that it can be done later in a different PR.
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 like @damiannolan's idea!