client/trade: Return counterPartyConfirms error. #1595
Conversation
|
I really though I was on to something and reproducing #1561 , but it turns out it was just this. I am able to trade with short locktimes now. Yet unable to cause the init failures on demand. Or maybe it's just fixed for some reason. |
JoeGruffins
force-pushed
the
donterrorifnotfound
branch
from
7bcb0ec
to
0b246db
Compare
|
Interesting. Makes perfect sense. I think I made this bug BTW. You must have been seeing Thoughts if this bug can affect btc/dcr etc or just eth? Might need to put this bug fix in 0.4.3 since 79ccdd8 is on that branch. EDIT: looks like only with eth does it actually have to find the swap for |
| if !errors.Is(err, asset.ErrSwapNotInitiated) { | ||
| // No need to log an error if swap not initiated as this | ||
| // is expected for newly made swaps involving contracts. | ||
| t.dc.log.Errorf("Failed to get confirmations of the counter-party's swap %s (%s) "+ |
There was a problem hiding this comment.
The static text part of this error is now mostly redundant since caller now has t.dc.log.Errorf("isSwappable: unable to get counter party confirms: %v", err) (or a variation with isRedeemable).
There was a problem hiding this comment.
Removed some text from the caller's log...
JoeGruffins
force-pushed
the
donterrorifnotfound
branch
from
0b246db
to
07c1732
Compare
|
Just rebased. |
Because the return data cannot all be trusted if the method has an error, return that error up to the caller rather than just logging and returning with possibly false values. For swaps using contracts it is ok for the swap to be status none, in which case we cannot check the lock time or confirmations on chain yet. Do not log an error in this case.
JoeGruffins
force-pushed
the
donterrorifnotfound
branch
from
07c1732
to
877d5ef
Compare
| coin, t.wallets.toAsset.Symbol, match, t.UID(), err) | ||
| } | ||
| return | ||
| return fail(fmt.Errorf("failed to get confirmations of the counter-party's swap %s (%s) "+ | ||
| "for match %s, order %v: %w", | ||
| coin, t.wallets.toAsset.Symbol, match, t.UID(), err)) |
There was a problem hiding this comment.
Should we only return an error if !errors.Is(err, asset.ErrSwapNotInitiated)? Otherwise, just call it zero confs. That would capture the behavior from before, and this part doesn't appear to be related to the bug you're fixing.
There was a problem hiding this comment.
Makes sense, but the LocktimeExpired call above would have to error on ErrSwapNotInitiated because the zero-value for lockTime returned on error would be interpreted as "expired". Which means it would never be ErrSwapNotInitiated down here because it would have been so above (for ETH at least).
An alternative might be to change the expired logic to expired = !lockTime.IsZero() && time.Until(lockTime) < 0 so it never says expired=true when the lockTime is the zero value as is the case when LocktimeExpired has an error. But I think I agree with Joe's conclusion that bubbling up an error is the least error prone way to keep the caller from jumping to conclusions about the actual state of the contract when it is actually unknown.
If you have a specific diff @buck54321 we could talk over that? I would like to get this fix in today in one form or another, and we could push a commit onto this PR if there's a revision that looks good.
There was a problem hiding this comment.
Makes sense, but the
LocktimeExpiredcall above would have to error onErrSwapNotInitiated
I'm not suggesting changing anything up there. Just here to match previous behavior that wasn't broken.
There was a problem hiding this comment.
I understand, but I'm saying "Which means it would never be ErrSwapNotInitiated down here because it would have been so above (for ETH at least)."
There was a problem hiding this comment.
Sorry I must be misunderstanding your comment "only return an error if !errors.Is(err, asset.ErrSwapNotInitiated)". Because when I mentally apply the suggestion, it would return for any error like it is in the present revision of this PR because it can't be ErrSwapNotInitiated because we're now catching it above (same contract call inside wallet.LocktimeExpired).
Apologies if I'm being dense.
There was a problem hiding this comment.
I see what you're saying now. The ErrSwapNotInitiated check down here is pointless then.
There was a problem hiding this comment.
Yea, eth should have already failed. I was going to do something like that at first, but after I was about to submit that pr I decided to do a little more that seems right to me and also fixes the bug. The returns should not be trusted if their is an error checking any of them, and the caller should be informed as of the reason of failure. The failure isn't that we are past locktime, the failure is that we couldn't check the locktime.
There was a problem hiding this comment.
imo was an easy bug to happen because of method design. Seeing the same thing in #1599 should just return errors and nil values on failure.
Checking the eth locktime would fail because expire was set to true even though checking the locktime failed. This error was not as helpful as the real error would have been. Also, there is no reason to log an error if the swap has not been mined because we cannot know anything about the counter party's swap without the on chain data.