imp: create and reuse errors in core channel upgrades handlers #5389
Conversation
vishal-kanna
requested review from
colin-axner,
AdityaSripal and
damiannolan
as code owners
|
Hi, it looks like there's some compiler errors here from a redeclared function. I would also suggest naming the errors - invalidupgraderoute := errorsmod.Wrapf(porttypes.ErrInvalidRoute, "upgrade route not found to module: %s", module)
+ err = errorsmod.Wrapf(porttypes.ErrInvalidRoute, "upgrade route not found to module: %s", module) |
crodriguezvega
requested review from
crodriguezvega,
charleenfei,
chatton and
srdtrk
as code owners
|
cleaned the history up since it was bringing in 350 commits with a 25k diff 😅. should ideally look clean and reviewable now ✨ |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #5389 +/- ##
==========================================
- Coverage 81.27% 81.20% -0.08%
==========================================
Files 197 197
Lines 15234 15235 +1
==========================================
- Hits 12382 12372 -10
- Misses 2391 2400 +9
- Partials 461 463 +2
|
It is uneeded if the if guard fails.
| @@ -856,19 +856,22 @@ func (k Keeper) ChannelUpgradeAck(goCtx context.Context, msg *channeltypes.MsgCh | |||
|
|
|||
| app, ok := k.Router.GetRoute(module) | |||
| if !ok { | |||
| ctx.Logger().Error("channel upgrade ack failed", "port-id", msg.PortId, "error", errorsmod.Wrapf(porttypes.ErrInvalidRoute, "route not found to module: %s", module)) | |||
| return nil, errorsmod.Wrapf(porttypes.ErrInvalidRoute, "route not found to module: %s", module) | |||
| err = errorsmod.Wrapf(porttypes.ErrInvalidRoute, "route not found to module: %s", module) | |||
There was a problem hiding this comment.
we could reinitialise new errors here as they'd be within the scope of the if, but I don't really feel super strong on it and happy to reuse err
There was a problem hiding this comment.
will push it in as is for now, can't say I have a strong pref either way too (slight preference for yours since it scopes values nicely but not enough pref to make the change)
| } | ||
|
|
||
| err = k.ChannelKeeper.ChanUpgradeAck(ctx, msg.PortId, msg.ChannelId, msg.CounterpartyUpgrade, msg.ProofChannel, msg.ProofUpgrade, msg.ProofHeight) | ||
| if err != nil { | ||
| ctx.Logger().Error("channel upgrade ack failed", "error", errorsmod.Wrap(err, "channel upgrade ack failed")) | ||
| errChanUpgradeFailed := errorsmod.Wrap(err, "channel upgrade ack failed") |
There was a problem hiding this comment.
any reason not to reuse err here instead of creating a new var with a different name?
err = errorsmod.Wrap(err, "channel upgrade ack failed") should work fine too, right?
There was a problem hiding this comment.
@damiannolan Using the err works fine.
There was a problem hiding this comment.
ah it won't. IsUpgradeError is a direct cast to UpgradeError, wrapping causes it to fail. (I think Cian mentioned this in a call at some point).
damiannolan
changed the title
|
do we backport to 8.1? cc @crodriguezvega |
imp: create and reuse errors in core channel upgrades handlers --------- Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> Co-authored-by: Cian Hatton <cian@interchain.io> (cherry picked from commit b68c4aa)
#5557) imp: create and reuse errors in core channel upgrades handlers --------- Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> Co-authored-by: Cian Hatton <cian@interchain.io> (cherry picked from commit b68c4aa) Co-authored-by: Vishal Potpelliwar <71565171+vishal-kanna@users.noreply.github.com>
closes #5378