Skip to content
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: Babysit refund transactions. #3082

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

JoeGruffins
Copy link
Member

@JoeGruffins JoeGruffins commented Nov 17, 2024

core: Confirm refund txs.

client/asset: Add confirm tx.

client/db: Upgrade to version seven.

closes #3063

@JoeGruffins JoeGruffins changed the title Babysitrefund client: Babysit refund transactions. Nov 17, 2024
client/asset/interface.go Outdated Show resolved Hide resolved
@JoeGruffins JoeGruffins force-pushed the babysitrefund branch 5 times, most recently from 31a4c6b to c6f14c0 Compare November 23, 2024 02:58
@JoeGruffins JoeGruffins force-pushed the babysitrefund branch 3 times, most recently from a50edc8 to 5a4b56a Compare November 26, 2024 08:47
@JoeGruffins JoeGruffins marked this pull request as ready for review November 26, 2024 08:47
@dev-warrior777
Copy link
Contributor

Looks good!

@buck54321 buck54321 requested a review from martonp November 28, 2024 00:25
client/asset/btc/btc.go Outdated Show resolved Hide resolved
client/asset/interface.go Outdated Show resolved Hide resolved
Comment on lines 382 to 392
amb := []byte("matches") // archived matches

var nRefunded int

defer func() {
upgradeLog.Infof("%d inactive refunded matches set to confirmed status", nRefunded)
}()

archivedMatchesBkt := dbtx.Bucket(amb)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
amb := []byte("matches") // archived matches
var nRefunded int
defer func() {
upgradeLog.Infof("%d inactive refunded matches set to confirmed status", nRefunded)
}()
archivedMatchesBkt := dbtx.Bucket(amb)
var nRefunded int
defer func() {
upgradeLog.Infof("%d inactive refunded matches set to confirmed status", nRefunded)
}()
archivedMatchesBkt := dbtx.Bucket(activeMatchesBucket)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to avoid using any outside variables in the upgrade, that way changes to surrounding code will not affect it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I should also apply this to matchKey and proofKey...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or did you mean to change to activeMatchesBucket? I don't think those can be revoked yet?

client/db/bolt/upgrades.go Outdated Show resolved Hide resolved
client/db/bolt/upgrades.go Outdated Show resolved Hide resolved
client/db/bolt/upgrades.go Outdated Show resolved Hide resolved
client/core/notification.go Outdated Show resolved Hide resolved
if _, is := t.accountRefunder(); is {
feeSuggestion = t.metaData.MaxFeeRate
} else {
feeSuggestion = t.redeemFeeSuggestion.get()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case would be incorrect, since it's using the fee suggestion for the redeem asset.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah... I think I meant to come back to this. I guess doing the same as for redeems.

client/core/trade.go Show resolved Hide resolved
@JoeGruffins JoeGruffins force-pushed the babysitrefund branch 2 times, most recently from 9dbd211 to 358a1bb Compare November 29, 2024 08:58
@JoeGruffins
Copy link
Member Author

Copy link
Contributor

@martonp martonp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been testing and I see a few issues:

  • When I do ETH/BTC swap, the side that is redeeming BTC cannot confirm the redemption:
Screenshot 2024-11-30 at 10 24 56 AM
  • When force a refund for eth, and then replace the transaction with another one with the same nonce, the popup for resubmitting the refund does not show up. If I restart bisonw, it shows up though. I tested this by using the test which I updated here: 0fe42e5 . You can update the seed and the nonce of the transaction that you want to have replaced, then run the test while waiting for the refund. Then you press enter right as the refund is submitted, and it will send a transaction with the same nonce, but a higher fee.

client/db/bolt/upgrades.go Show resolved Hide resolved
@JoeGruffins
Copy link
Member Author

@martonp Sorry I didnt rebase before pushing changes. The first point is probably fixed by the change in trade.go here https://github.com/decred/dcrdex/compare/358a1bb86d500748e189738b2808244a3618e1e6..39ee55b05b201f3c2d85c9558ed8624c1bd56e6c

I tried testing the replace transactions but I'm not sure if I'm blocking something bisonw needs for popups... I'll try some more.

@JoeGruffins
Copy link
Member Author

JoeGruffins commented Dec 4, 2024

Oh yeah, I'm only seeing the popup after restarting. hmmm...

@JoeGruffins
Copy link
Member Author

@martonp it is based on this timeout for a running instance

// txAgeOut is the amount of time after which we forego any tx
// synchronization efforts for unconfirmed pending txs.
txAgeOut = 2 * time.Hour

The tx should become stale and you will see the popup after that interval. You can reduce it ofc to check.

Comment on lines 766 to +769
ActionIDRedeemRejected = "redeemRejected"
TopicRedeemRejected = "RedeemRejected"
ActionIDRefundRejected = "refundRejected"
TopicRefundRejected = "RefundRejected"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the UI the action was updated to txRejected, so when these actions are sent, there is a UI error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, thanks. I guess keeping two here as handleCoreAction needs them to be different. Reverting the js change and adding refundRejected

@JoeGruffins
Copy link
Member Author

Copy link
Contributor

@martonp martonp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something is broken with both redemption and refund resubmission on ETH, even after I fix the bugs in the comments. Either the wrong nonce is used, or the transactions fail (status of transaction in receipt is 1).

const div = this.page.rejectedRedemptionTmpl.cloneNode(true) as PageElement
txRejectedAction (req: ActionRequiredNote) {
const { orderID, coinID, coinFmt, assetID, txType } = req.payload as RejectedTxData
const div = this.page.rejectedTxTmpl.cloneNode(true) as PageElement
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is updated to rejectedTxTmpl, but the html is not updated. Also the tx type label below needs to be added to the html.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure I updated this but it looks like I didn't commit changes.

@@ -10712,23 +10712,31 @@ func (c *Core) handleRetryRedemptionAction(actionB []byte) error {
coinID = match.MetaData.Proof.MakerRedeem
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

coinID needs to be the refund coin if this is retrying a refund.

@JoeGruffins
Copy link
Member Author

It's asking me twice now to replace. Something is off.

@JoeGruffins
Copy link
Member Author

transactions fail (status of transaction in receipt is 1).

Failure is 0x00 status I think?

@martonp
Copy link
Contributor

martonp commented Dec 23, 2024

This should fix the issue: 8d03288

@JoeGruffins
Copy link
Member Author

JoeGruffins commented Dec 24, 2024

Trying with the new harness changes and the thing mines a block if there is any transaction without telling it to. So, unable to get the replacement transaction in now.

@JoeGruffins
Copy link
Member Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

core: Babysit refund transactions.
3 participants