-
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
[for discussion] eth: locked amount accounting updates #1289
Conversation
The string address can have different formattings, so parse the input string into a common.Address and compare that to the account address. Also use direct array comparison for address equality instead of bytes.Equal.
return nil, nil, 0, asset.ErrNotImplemented | ||
} | ||
|
||
// Redeem sends the redemption transaction, which may contain more than one | ||
// redemption. | ||
func (*ExchangeWallet) Redeem(form *asset.RedeemForm) ([]dex.Bytes, asset.Coin, uint64, error) { | ||
// TODO: remember to unlockFunds for the reserved redeem gas amount. |
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.
When would those funds get locked in the first place?
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.
This is an open question, but as a thought, I made a comment https://github.com/decred/dcrdex/pull/1289/files#diff-671406d18db2f8e1a281545be05fcdefd789b63614398d678d38e2b2814cf788R549 FundBuyOrder
type coin struct { | ||
id srveth.AmountCoinID | ||
srveth.AmountCoinID |
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'm updating this struct in #1248, so there's no point to do this unless you disagree with those changes.
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.
That's fine. I'm not going to ram this PR through, just thinking through the fund locking and nonce issue.
|
||
cachedInitGas uint64 | ||
cachedInitGasMtx sync.Mutex | ||
|
||
lockedFunds map[string]uint64 // gwei |
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 discussed having the map vs just an integer over here:
#1221 (comment)
I don't think there's any benefit other than serving as a safety check, but I don't think there's any downside either.
Having the lockedAcct
struct is definitely nicer though in case we decide to support multiple addresses.
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 recall that discussion, and the earlier one about adding the nonce in the first place. #1221 (comment)
I'm still not convinced the more complicated coin id encoding helps much, and I'm trying to get to the bottom of it in the PR, which is just for discussion at this point.
The server-side considerations in that discussion are irrelevant now. Server will just check account balance. Both the nonce and amount in the coin ID are of no value to server. On the other hand, client (presently) only needs an amount. We're tracking the account too, thinking we'll have multiple accounts at some point, but even that is mostly dead weight client-side presently (they all match eth.acct.Address
and we ensure that).
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 I'm not opposed to removing this map either tbh, I think it might be more trouble than it's worth.
// selected coins, but since there are no redeem scripts in Ethereum, nil is returned. | ||
// Equal number of coins and redeem scripts must be returned. | ||
// TODO: "fund" a buy order by locking funds for the redeem transaction gas. | ||
// func (eth *ExchangeWallet) FundBuyOrder(ord *asset.Order) { do not use ord.Value, just MaxSwapCount } |
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.
This doesn't need to be part of every asset's interface, just a method to assert for at runtime to see if it's necessary.
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 think this will work.
// retired (no more swaps and no longer booked). The change amount is the input | ||
// amount minus the actual amount *spent* by this swap txn. |
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.
@martonp When diving into this, I was considering your Swap PR. Namely ee847d0#diff-671406d18db2f8e1a281545be05fcdefd789b63614398d678d38e2b2814cf788R831-R838.
I believe this is actually a way it's good to just have amounts and not unique coin ids keying in a map of values. Instead of unlockFunds(inputs)
followed by lockFunds(new fake coin for change)
, it would just be unlockFunds(totalUsedValue)
I do not intend to front run #1248, but I think things can eventually work this way and simplify a lot of things.
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 anyone is very adamant about keeping the map, might as well front run that PR and update this.
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.
Whatever we do, I did mean to imply that #1248 should go first. I'm not trying to jump the line. :)
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.
@buck54321 @JoeGruffins What's your opinion on this?
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 anxious to see where chappjc takes this. I do think the idea of discrete "coins" is of little value server-side, but could provide a convenient way for account client-side. Maybe just a quick thought experiment.
We'll call the counter method A and the coin method B.
Let's say a multi-lot order requires 100 units max. We'll lock up that value by either
A) incrementing the counter by 100 units. new value 100 units.
B) creating a coin with an associated value of 100 units and adding it to the map
OK. Time to swap.
Swap(*Swaps) (receipts []Receipt, changeCoin Coin, feesPaid uint64, err error)
A) I guess we just don't pass a "coin" in the Swaps.Inputs
. If we're not going to put a nonce in, and the value isn't necessarily unique, what's the purpose of a coin? We decrement the counter by the amount sent. Not sure how we track the remainder. Maybe Core
needs special handling. And what if this is the last swap. We will always have a remainder from original locked amount. How is that remainder tracked and returned after the order completes?
B) Decrement the amount decoded from the Swap.Inputs, create a new coin with the decremented value and return it as the change coin. If this is the last swap in the order, Core indicates that with the Swaps.LockChange
field, and we remove the coin from the map.
@chappjc's obviously got something working and I'm sure there's a lot I'm not seeing, but from where I'm at, it looks like "coins" make a good accounting tool client-side and fit very nicely in the existing core system with virtually no changes.
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) I guess we just don't pass a "coin" in the
Swaps.Inputs
. If we're not going to put a nonce in, and the value isn't necessarily unique, what's the purpose of a coin? We decrement the counter by the amount sent. Not sure how we track the remainder. MaybeCore
needs special handling. And what if this is the last swap. We will always have a remainder from original locked amount. How is that remainder tracked and returned after the order completes?
Here it would still use a changeCoin here to convey the remainder amount that needs to be unlocked later. That won't need to change. Everything is in place with trackedTrade.{coin,change}
for this in Core, and we rely on it presently, so I don't believe there's really much if any change on the Core machinery. That's the NOTE
we're commenting on now. :) I think it really is identical to (B).
In ETH on the client, the only purpose of a Coin for order funding is the value, at least until/if we have multiple account support.
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.
So we still have discrete "coins", but we just use them to know how much to change the counter rather than storing them in a map? That should work fine.
And since we don't need a map key, I guess the coins don't need a nonce. We can just trust that core is handling them properly.
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.
Yes, that's the idea. We don't need to get away from the trackedTrade.{coin,change}
workings at all, we just don't need a nonce. I convinced myself this was fine in reviewing the Core bits around the trackedTrade
and returncoins.
Anyway, I think I'm just going to let this PR sit until a number of other ETH PRs are in first, but I wanted to discuss this asap because I had a terrible itch regarding the nonce business.
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 original reason for the nonce was to make the signed messages that are sent to the server unique.
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 original reason for the nonce was to make the signed messages that are sent to the server unique.
I'm glad you're pointing this out @martonp, because that was never really an issue to solve I believe, at least not with the coin ID. You get reused UTXOs commonly too #1221 (comment) If we want to guard against order signature replays, we need a different solution entirely. Perhaps instead of using the coin ID as the message being signed we sign the serialized Trade, although that could be replayed too, at least until the client time clock deviation threshold is exceeded.
And now with @buck54321's account-balance-based solution in #1293, the server has no need for them to be unique for the locking task.
There were a few things I thought the nonce helped with client side. One is persisting the locked coins in the db. But I guess there's no reason to do this, as the lock coins are recalculated during resumeTrades. The other is to make sure you aren't locking or unlocking the same coins for the same trades. I would think it's easier to do accounting with a way to know what values are locked for what trade, but I guess it's comparable to eth's own accounting, where you forget what txs led up to an account's state. Although, they do have an incremented nonce. Maybe the trade and match trackers will ensure that nothing is double locked or unlocked. But I do think troubleshooting would be easier, if a strange amount was locked, and the amount had something that connected it to an order or match, or why it's locked. Unique coin id's were helpful when trying to figure out why more coins were being locked for dcr in the simnet tests recently. I was able to pinpoint that it was happening with the taker's coins partly because of unique coin ids. |
I'm still baffled by the nonce, but we'll figure it out |
No description provided.