-
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/eth: FundOrder, ReturnCoins, FundingCoins #1221
Conversation
b1a2bf2
to
7cf80b8
Compare
7cf80b8
to
91578d6
Compare
client/asset/eth/eth.go
Outdated
func (c *EthCoin) ID() dex.Bytes { | ||
id := make([]byte, ethCoinIdLength) | ||
copy(id[:common.AddressLength], c.address.Bytes()) | ||
binary.BigEndian.PutUint64(id[common.AddressLength:], c.value) | ||
return id | ||
} |
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 interesting problem. We're technically already communicating the value via the Value
method, and the address is accessible, and already part of the msgjson.Trade
, so even if we returned no coins, the server would still have what it needs, and could calculate the amount locked using same math you are,
ord.Value + ord.DEXConfig.MaxFeeRate * ord.DEXConfig.SwapSize * ord.MaxSwapCount
. This would, of course, require the server to know the difference between account- and utxo-based assets, but we know that we need that awareness in the CoinLocker
anyway.
On the other hand, we do need to demonstrate ownership of the private key associated with the funding address, and that validation is connected with the Coin
, so having a single coin makes sense in that respect. And we would need the value encoded since methods like FundingCoins
would have no other way of attaining it.
The only other issue I see is that there are likely to be a lot of duplicate coin IDs, because two orders of the same size will have the same values = coin IDs. We are signing the coin IDs as part of order validation, and they should really be unique.
A simple solution to this would be to add a random nonce to the coin IDs. I think this would also provide some benefits system-wide since we can expect uniqueness again.
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 only other issue I see is that there are likely to be a lot of duplicate coin IDs, because two orders of the same size will have the same values = coin IDs. We are signing the coin IDs as part of order validation, and they should really be unique.
That's an interesting point. But with UTXO assets, an order X can be funded with coin A, order X canceled, and another order Y funded with the same coin A again. Did you identify a bigger issue I wonder?
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 address is accessible, and already part of the
msgjson.Trade
, so even if we returned no coins, the server would still have what it needs
Sorry, I haven't thought about msgjson.Trade
recently so I could have this backwards, but if you were selling ETH for BTC, your msgson.Trade.Address
would be a BTC address (from the "to" wallet), while the Coins would be for ETH (from the "from" wallet), right?
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 address is accessible, and already part of the
msgjson.Trade
, so even if we returned no coins, the server would still have what it needsSorry, I haven't thought about
msgjson.Trade
recently so I could have this backwards, but if you were selling ETH for BTC, yourmsgson.Trade.Address
would be a BTC address (from the "to" wallet), while the Coins would be for ETH (from the "from" wallet), right?
Oh, yeah. Good point.
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 only other issue I see is that there are likely to be a lot of duplicate coin IDs, because two orders of the same size will have the same values = coin IDs. We are signing the coin IDs as part of order validation, and they should really be unique.
That's an interesting point. But with UTXO assets, an order X can be funded with coin A, order X canceled, and another order Y funded with the same coin A again. Did you identify a bigger issue I wonder?
Nothing serious. We are keying on coin ID in e.g. trackedTrade.coins
, but as long as we only return a single coin per order, that shouldn't be a problem. AssetCoinLocker.lockedCoins
is another place where coin ID is used as a unique key, though tons of changes are needed there regardless.
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 really need to think about the nonce. Having the same data represented with different (random) bytes is going to be a PITA and confusing in a host of other ways I'm almost certain.
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.
Could do a timestamp instead.
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 haven't thought it through.. we can run with what you have
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 don't think a timestamp would be good, because possible to get two of the same.
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 like the nonce.
b3ccda4
to
b066eed
Compare
Ok, I'm a little embarrassed, but I just realized server/asset.Coin and client/asset.Coin are two different things. Hope I didn't make things too awkward. |
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 was afraid I led you astray saying put all the coin id's with the server, but actually looks good imo.
I think that maybe the locked value should be stored in the database, but maybe should be another pr if so. External utxo wallets, you tell them to lock the coins and even if you restart dexc I don't think that will change anything there. So I think we also need a way to persist the locked amount in case of shutdown while trading, on purpose or maybe a power failure, some bug. We would need to know and set the locked amount on start-up. Could even just cycle through active trades and create the value without any db changes I guess, since there is only the one account at the moment. That might stay that way forever as well.
Anyway, I think the current changes are a solid step in the right direction.
client/asset/eth/eth.go
Outdated
@@ -153,6 +158,9 @@ type ExchangeWallet struct { | |||
currentTip *types.Block | |||
|
|||
acct *accounts.Account | |||
|
|||
lockedFunds 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.
I wonder if this would actually be better as slice or map of coin ID? That way you would also be sure you weren't locking/unlocking the same coin twice, if such a thing is possible. Not sure if it would be a benefit now, but if at some point we did use multiple accounts/addresses it would be useful.
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 wouldn't work because we assign a random nonce to each coin, so coin IDs will all be unique. I think if we support multiple addresses, then we can just do an address -> uint64
map.
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 wouldn't work because we assign a random nonce to each coin, so coin IDs will all be unique
That's why it would work. Before the nonce, we didn't have anything to key on. Now we do. I like the idea of a map[string]uint64
or similar, where the key is hex.EncodeToString(coinID)
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.
Now that I think about it more I guess are some benefits to having the map. I was thinking that there's no reason for the map because on every call to FundOrder
, a new coin is created, so there would never be a case where two orders are funded by the same coins. However when ReturnCoins
is called, we could avoid decreasing the locked amount if ReturnCoins
is called with a coin that is not in the map. This could happen if ReturnCoins
is called twice with coins from the same order for some reason. I don't think the map really serves a purpose if there are no bugs in the rest of the codebase, but might as well have it as a safety check.
This PR implements locking in the ETH wallet. Since there are no "coins" in the ETH account system as there are in UTXO based coins, the amount of ETH that has been reserved for orders is kept track of in the ETH wallet. This PR also includes a refactoring of the CoinID code in the server. The changes in the client code needed a new coin id type which represented coins in an address that had not yet been spent.
// AmountCoinID is an identifier for a coin which has not yet been sent to the | ||
// swap contract. | ||
type AmountCoinID struct { | ||
Address common.Address | ||
Amount uint64 | ||
Nonce [8]byte | ||
} |
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 type will also be used for server.FundingCoins since we are trying to portray? unspent value on the blockchain there as well, so glad it's here after all.
I'm going to go ahead and rebase off of this pr.
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 that's why I wanted it to be accessible from both the server/client. The client sends these IDs to the server.
Part of #1154