refactor: use branch service in 29-fee #7732
Conversation
| for _, packetFee := range packetFees { | ||
| if !k.EscrowAccountHasBalance(ctx, packetFee.Fee.Total()) { | ||
| // NOTE: we lock the fee module on error return so that the state changes are persisted | ||
| return ibcerrors.ErrInsufficientFunds |
There was a problem hiding this comment.
I understand the concept here, but I have to admit I don't love the use of errors as control flow. It feels a lot like using exceptions as control flow and while it works, it is quite ugly. Another potential way to do this would be to not shadow the ctx variable so that you could write to state directly here and then return an error. Of course, that has some downsides too as it makes it more likely to accidentally use the wrong context inside the here (and it feels weird to write to state inside the branch callback).
Not sure which I prefer, they are both ugly to me :D
There was a problem hiding this comment.
Could another potential solution be to just do this loop before branching? We only really need it for the last line, right?
There was a problem hiding this comment.
I agree with your rational in general. I don't really love the use of errors as control flow per se as well.
On the other hand this is an exceptional circumstance, and in most code flows we will likely not be needing partial state changes and such, so I would be fine to keep it like this.
I kinda like the idea of not shadowing the ctx var and seeing how that behaves - in theory I think it should(?) work. But I also think its less explicit than what we are doing here already and having clear and readable code is more preferable imo when dealing with funds.
Could another potential solution be to just do this loop before branching? We only really need it for the last line, right?
This could potentially be an opt also maybe. But I'd like to stray from spending too much time on this rn given how much this module is used and its lifespan going forward.
|
Description
This PR migrates the 29-fee module to use
env.BranchServicein favour of cached context.ref: #7712
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/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.