core,eth: handle redemption funding #1314

buck54321 · 2021-11-29T22:54:42Z

    client/asset:
    Add AccounterRedeemer interface with methods for locking and unlocking
    funds for redemption. ReserveN is used to reserve funds for an order.
    Only the amount reserved is returned. All accounting from there is
    handled by Core, who will return funds proportionally according to
    match quantities and order size. The amount reserved is persisted in
    the OrderMetaData so that accounting can be picked up exactly as it was on restarts.
    This pattern was chosen over trying to figure out how much to lock
    on restart with calculations based on order info because you would need
    to consider changes in asset configuration and therefore persist more
    data and ... it gets messy.
    
    client/core:
    Add RedeemSig to orders redeeming to and AccountRedeemer.
    Track and return reserved redemption funds. Return funds manually
    for refunds and revocations. Handle dust that can arise from rounding
    error. Log inconsistencies.
    
    client/asset/eth:
    Implement asset.AccountRedeemer. Track redemption reserves separately
    than swap reserves, but add them when calculating locked balance.

chappjc · 2021-11-29T23:18:26Z

I was imagining a FundBuyOrder to mirror the FundOrder method, which is implicitly for a sale of the asset. Then just use the usual ReturnCoins if cancelling or executing unfilled. Does splitting the redemption reserves away from other locked coins solve a major challenge with that approach?

buck54321 · 2021-11-29T23:28:08Z

I was imagining a FundBuyOrder to mirror the FundOrder method, which is implicitly for a sale of the asset. Then just use the usual ReturnCoins if cancelling or executing unfilled. Does splitting the redemption reserves away from other locked coins solve a major challenge with that approach?

Not 100% sure I follow, but you should try it out. This is maybe the third different direction I started down, and the others got messy.

I think FundBuyOrder might be confusing though, since it's presumably still used for sell orders (toAsset = sell & quote || buy && base).

chappjc · 2021-11-30T00:12:52Z

I will consider it more. I do appreciate some of the issues you've addressed by tracking amounts instead of coins or lots independently.
Perhaps FundBuyOrder would be better as FundReceivingOrder, but it's probably a moot point.

client/asset/eth/nodeclient.go

buck54321 · 2021-12-22T14:34:33Z

Still need match revocation handling, but if nobody objects, I'll work on getting this out of draft.

chappjc · 2021-12-22T14:35:49Z

OK.

…

On Wed, Dec 22, 2021, 8:34 AM buck54321 ***@***.***> wrote: Still need match revocation handling, but if nobody objects, I'll work on getting this out of draft. — Reply to this email directly, view it on GitHub <#1314 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACHQOSICJKESRAGXCFHKH2LUSHOYJANCNFSM5JAJPS6Q> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you commented.Message ID: ***@***.***>

buck54321 · 2022-01-05T18:06:12Z

Order Completion

Result	Action
Revoked before matching	return all [1]
Revoked from booked	return remaining [1]
Canceled	return remaining [2]
nomatch - market order	return all [3]
Executed - market buy	do nothing. will return when redeemed
Executed - market sell	return remaining [2]
Executed - limit	nothing

Match Revoked

Step	Maker	Taker
NewlyMatched	return all [4]	return all [4]
MakerSwapCast	nothing (will refund) [5]	return all [4]
TakerSwapCast	nothing (will redeem) [6]	nothing (will redeem via findRedemption, or refund) [5]
MakerRedeemed	nothing	nothing (will redeem) [6]

client/asset/eth/eth.go

JoeGruffins · 2022-01-07T03:24:08Z

client/asset/eth/nodeclient.go

+	pubKey, err = recoverPubkey(crypto.Keccak256(data), sig)
+	if err != nil {
+		return nil, nil, fmt.Errorf("SignMessage: error recovering pubkey %w", err)
+	}


I suppose we could get the pubkey at start-up and there would not be a need to recover every time? But if we ever have more than one address being used, it will be needed, I guess.

I actually had it that way at one point.

JoeGruffins · 2022-01-07T03:30:20Z

client/asset/interface.go

@@ -227,6 +227,20 @@ type Wallet interface {
 	RegFeeConfirmations(ctx context.Context, coinID dex.Bytes) (confs uint32, err error)
 }

+type AccountRedeemer interface {


Looking at how funding swaps will work, they will also need special care, and I guess will need a similar interface. For example, the entire funding msg is signed rather than just the funding coin ids. Could the naming here be changed to include more than just redeems? Maybe just Accounter or is that wierd.

Also, could use a doc above.

I assume you mean the problems you're addressing in #1392? I don't think we'll need any new Wallet methods for swap funding.

client/core/core.go

martonp

LGTM, just minor comments

client/core/trade.go

client/core/core.go

client/asset: Add AccounterRedeemer interface with methods for locking and unlocking funds for redemption. ReserveN is used to reserve funds for an order. Only the amount reserved is returned. All accounting from there is handled by Core, who will return funds proportionally according to match quantities and order size. The amount reserved is persisted in the OrderMetaData so that accounting can be picked up exactly as it was on restarts. This pattern was chosen over trying to figure out how much to lock on restart with calculations based on order info because you would need to consider changes in asset configuration and therefore persist more data and ... it gets messy. client/core: Add RedeemSig to orders redeeming to and AccountRedeemer. Track and return reserved redemption funds. Return funds manually for refunds and revocations. Handle dust that can arise from rounding error. Log inconsistencies. client/asset/eth: Implement asset.AccountRedeemer. Track redemption reserves separately than swap reserves, but add them when calculating locked balance.

buck54321 force-pushed the redeem-sig branch from b421d51 to bf2d28e Compare December 6, 2021 23:00

chappjc mentioned this pull request Dec 7, 2021

eth/client: Redeem #1302

Merged

JoeGruffins mentioned this pull request Dec 8, 2021

eth/client: Don't allow submitting a trade if not enough balance to redeem #1275

Closed

buck54321 force-pushed the redeem-sig branch from bf2d28e to 20583a2 Compare December 8, 2021 16:20

chappjc reviewed Dec 17, 2021

View reviewed changes

client/asset/eth/nodeclient.go Outdated Show resolved Hide resolved

chappjc mentioned this pull request Dec 22, 2021

client/asset/eth: Add RecoverPubkey for no-cgo. #1358

Merged

buck54321 force-pushed the redeem-sig branch 2 times, most recently from f47c250 to c919f53 Compare December 22, 2021 16:04

buck54321 force-pushed the redeem-sig branch from c919f53 to 8abbe51 Compare December 31, 2021 01:55

JoeGruffins mentioned this pull request Jan 5, 2022

client/eth: Sign redeems. #1384

Closed

buck54321 force-pushed the redeem-sig branch from 8abbe51 to 28add9f Compare January 5, 2022 17:56

buck54321 force-pushed the redeem-sig branch 5 times, most recently from 85764f3 to bee8fc3 Compare January 6, 2022 12:51

buck54321 marked this pull request as ready for review January 6, 2022 12:52

JoeGruffins mentioned this pull request Jan 7, 2022

eth: Funding coin encoding. #1389

Closed

JoeGruffins approved these changes Jan 7, 2022

View reviewed changes

JoeGruffins mentioned this pull request Jan 7, 2022

eth: Funding coin encoding. #1392

Merged

martonp approved these changes Jan 7, 2022

View reviewed changes

client/core/trade.go Outdated Show resolved Hide resolved

client/core/core.go Outdated Show resolved Hide resolved

buck54321 force-pushed the redeem-sig branch from bee8fc3 to 9c3428a Compare January 10, 2022 09:57

review followup

6242cb0

buck54321 force-pushed the redeem-sig branch from 9c3428a to 6242cb0 Compare January 10, 2022 10:21

chappjc self-requested a review January 10, 2022 14:11

chappjc approved these changes Jan 11, 2022

View reviewed changes

chappjc merged commit e1218a2 into decred:master Jan 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core,eth: handle redemption funding #1314

core,eth: handle redemption funding #1314

buck54321 commented Nov 29, 2021 •

edited

Loading

chappjc commented Nov 29, 2021 •

edited

Loading

buck54321 commented Nov 29, 2021 •

edited

Loading

chappjc commented Nov 30, 2021

buck54321 commented Dec 22, 2021

chappjc commented Dec 22, 2021 via email

buck54321 commented Jan 5, 2022

JoeGruffins Jan 7, 2022

buck54321 Jan 10, 2022

JoeGruffins Jan 7, 2022

buck54321 Jan 10, 2022

martonp left a comment

core,eth: handle redemption funding #1314

core,eth: handle redemption funding #1314

Conversation

buck54321 commented Nov 29, 2021 • edited Loading

chappjc commented Nov 29, 2021 • edited Loading

buck54321 commented Nov 29, 2021 • edited Loading

chappjc commented Nov 30, 2021

buck54321 commented Dec 22, 2021

chappjc commented Dec 22, 2021 via email

buck54321 commented Jan 5, 2022

JoeGruffins Jan 7, 2022

Choose a reason for hiding this comment

buck54321 Jan 10, 2022

Choose a reason for hiding this comment

JoeGruffins Jan 7, 2022

Choose a reason for hiding this comment

buck54321 Jan 10, 2022

Choose a reason for hiding this comment

martonp left a comment

Choose a reason for hiding this comment

buck54321 commented Nov 29, 2021 •

edited

Loading

chappjc commented Nov 29, 2021 •

edited

Loading

buck54321 commented Nov 29, 2021 •

edited

Loading