-
Notifications
You must be signed in to change notification settings - Fork 92
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
client/core: Only check for fees when they are definitely available #2062
Conversation
The fee checks for dynamic fee assets were happening many times before the transaction was confirmed, unnecessarily slowing the ticks down.
client/core/trade.go
Outdated
return match.Status >= order.MakerSwapCast | ||
return match.Status > order.MakerSwapCast || (match.Status == order.MakerSwapCast && mine > 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.
Why not...
return match.Status >= order.MakerSwapCast && mine > 0
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.
Unless I'm mistaken the confirms()
value is all that matters now. If it's confirmed, does it matter what the match status is?
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 reason I did it like this is the confirmation updates seemed very irregular using the MultiRPC. On Etherscan I would see many confirmations while still showing 0 in the dex client. I was worried that maybe in some case setSwapConfirms
would not be called.
How about
return match.Status > order.MakerSwapCast || mine > 0
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.
On Etherscan I would see many confirmations while still showing 0 in the dex client. I was worried that maybe in some case
setSwapConfirms
would not be called.
In this case, surely the dynamic fee check would also fail to find it on the chain?
EDIT: I now see your point that it might never say >0 from confirms()
. #2062 (comment)
client/core/trade.go
Outdated
@@ -1386,10 +1386,13 @@ func (t *trackedTrade) checkSwapFeeConfirms(match *matchTracker) bool { | |||
// Confirmed will be set in the db. | |||
return true | |||
} | |||
// Waiting until the swap is definitely confirmed in order to not | |||
// keep calling the fee checker before the swap is confirmed. | |||
mine, _ := match.confirms() |
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.
A more descriptive variable would be nice. mySwapConfs
or something.
client/core/trade.go
Outdated
if match.Side == order.Maker { | ||
return match.Status >= order.MakerRedeemed | ||
} | ||
// Checking taker redemption fees might be a bit early, but there will be | ||
// no more ticks after the status is set to MatchConfirmed. | ||
return match.Status >= order.MatchComplete |
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's not clear what the intention is here. For maker, the difference between MakerRedeemed
and MatchComplete
is just a successful sendRedeemAsync
, which should come immediately after the status is set to MakerRedeemed
anyway, and seems unrelated to whether or not we want to check confirmations.
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.
Yeah you're right, this change is unnecessary. For the redemption I don't think there's anything we can do due to the irregular confirmation updates I mentioned #2062 (comment) . If one updates takes us from 0 to 10, and the status is set to MatchConfirmed
, tick
will no longer run. Unless we update matchIsActive
to take into account these fees.
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 believe this (keeping as-is on master for the redeem) is fine since unlike with the swap transactions, there should be no delays after the maker broadcasts their redeem txn.
if match.Side == order.Maker { | ||
return match.Status >= order.MakerSwapCast | ||
return match.Status > order.MakerSwapCast || mySwapConfs > 0 |
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.
Why >
instead of >=
? Why should maker wait to see the taker's swap before checking for fees on its own swap?
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's not waiting, if the maker's swap has 1 confirmation, this will return true. If we put >=
, then this will constantly check before it's ready, unnecessarily holding up tick
.
The match.Status > order.MakerSwapCast
is there just to be conservative in case the swap confirms doesn't get updated. We are not strictly tracking the confirmations on the swap transaction, and due to the irregular way the confirmations were coming in when testing on testnet, I thought this would be safer.
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 are not strictly tracking the confirmations on the swap transaction, and due to the irregular way the confirmations were coming in when testing on testnet, I thought this would be safer.
Hmm, that's an interesting point. I suppose there can be a case where no tick
gets your own swap's confirms before moving on to the next status, which could preclude the confs check.
OK, I think this is safest since tick does not make explicit guarantees about this.
Actually, I think the counter party could even make their transaction without waiting for a single confirmation on ours. Nothing stopping that.
The fee checks for dynamic fee assets were happening many times before the transaction was confirmed, unnecessarily slowing the ticks down.