add UpdateChannel method to Endpoint
#5725
Conversation
charleenfei
requested review from
colin-axner,
AdityaSripal,
damiannolan,
crodriguezvega,
chatton and
DimitrisJim
as code owners
charleenfei
changed the title
UpdateChannel method to Endpoint
There was a problem hiding this comment.
lgtm! 🚀 One thing I'd suggest is ideally going through tests and updating the current pattern of GetChannel -> modify -> SetChannel. Did that in connection case by grepping for the Set mostly (Get can be done for other reasons).
grep -inr "SetChannel" . --include="*_test.go" should find most!
testing/endpoint.go
Outdated
| @@ -772,6 +772,17 @@ func (endpoint *Endpoint) SetChannelState(state channeltypes.State) error { | |||
| return endpoint.Counterparty.UpdateClient() | |||
| } | |||
|
|
|||
| // UpdateChannelState updates a channel | |||
| func (endpoint *Endpoint) UpdateChannel(updater func(channel *channeltypes.Channel)) error { | |||
There was a problem hiding this comment.
do we want to mark SetChannelState as deprecated? We could say they'll be removed in v10. Can open issue to track it.
There was a problem hiding this comment.
yea, I can mark it as deprecated, good call :)
testing/endpoint.go
Outdated
|
|
||
| endpoint.Chain.Coordinator.CommitBlock(endpoint.Chain) | ||
|
|
||
| return endpoint.Counterparty.UpdateClient() |
There was a problem hiding this comment.
wonder if people would prefer this to be a require.NotNil instead of returning the error? We always seem to require this error not be nil when using it (unless I missed something in diff) and can't really think of why we'd want to actually handle the error if one is returned.
There was a problem hiding this comment.
I think having the Require set in the test makes more sense in the context of the naming of the function (unless we rename to something like MustUpdateChannel?), also then the condition is more explicit when you read the test itself?
but tbh not so opinionated, I'd be fine with updating this if other people find it better.
There was a problem hiding this comment.
yea, fine either way too. just feels weird always asserting no error here, can't see a case where we'd want to assert one
…hub.com:cosmos/ibc-go into charly/issue#3984-add-updatechannel-to-endpoint
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #5725 +/- ##
==========================================
- Coverage 81.55% 81.54% -0.01%
==========================================
Files 199 199
Lines 15180 15175 -5
==========================================
- Hits 12380 12375 -5
Misses 2334 2334
Partials 466 466
|
There was a problem hiding this comment.
ACK changes, thanks @charleenfei and @crodriguezvega for final push!
crodriguezvega
deleted the
charly/issue#3984-add-updatechannel-to-endpoint
branch
Description
closes: #3984
Commit Message / Changelog Entry
see the guidelines for commit messages. (view raw markdown for examples)
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/).godoccomments.Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.