dex,client/asset: add token primitives #1394
Conversation
| package dex | ||
|
|
||
| // Gases is a the gas required for various operations. | ||
| type Gases struct { |
There was a problem hiding this comment.
Unlike with the ETH swap contract where these values could only change due to a consensus rules change, these gas values could change if the developers of the ERC20 token change their implementation. The implementation can be changed by using a proxy delegate:
https://fravoll.github.io/solidity-patterns/proxy_delegate.html
It would be safer to call EstimateGas each time instead of using pre-calculated values.
There was a problem hiding this comment.
Hmm. Good point. I think it's good to estimate initiations at order time. I was actually considering doing an estimate for all swaps in FundOrder anyway (could replace the balance check, I think). We won't be able to estimate redeems and refunds at order time, so we'll need some estimates there.
There was a problem hiding this comment.
Yeah for redeem and refund we can't do estimates. If there is a nice buffer on top of the estimate, it's unlikely we'll run into any issues.
dex/networks/eth/params.go
Outdated
| }, | ||
| } | ||
|
|
||
| func VersionedNetworkToken(assetID uint32, contractVer uint32, net dex.Network) (*dex.Token, *dex.TokenAddresses, common.Address, error) { |
There was a problem hiding this comment.
Why do you need to return both the dex.TokenAddresses and also the specific address for the version requested?
client/asset/eth/eth.go
Outdated
| @@ -34,8 +34,32 @@ import ( | |||
| "github.com/ethereum/go-ethereum/crypto" | |||
| ) | |||
|
|
|||
| var ( | |||
| erc20AllowanceOption = &asset.ConfigOption{ | |||
There was a problem hiding this comment.
If you select limited allowance, would it do an approve transaction before each trade?
I was envisioning something where the user could explicitly do approve transactions on the wallet page for each token. It would show the current amount approved, then allow them to set unlimited, 0, or whatever amount they want.
There was a problem hiding this comment.
So for a user to buy a token, they'll need to go to the wallets page first and verify that they have enough allowance, and then manually adjust the allowance before going back to the orders page and placing the order?
There has to be a way for this to happen automatically. Users will not want to do this manually. If we want advanced options, we can offer those, but the default behavior should require no additional steps from the user.
There was a problem hiding this comment.
If we do an automated thing, we should just have the allowance be unlimited. Changing allowances is tricky business from what I've read, needing to set back to zero and then the new allowance in a separate txn. https://docs.openzeppelin.com/contracts/3.x/api/token/erc20#IERC20-approve-address-uint256-
https://docs.google.com/document/d/1YLPtQxZu1UAvO9cZ1O2RPXBbT0mooh4DYKjA_jp-RLM/edit
https://github.com/sec-bit/awesome-buggy-erc20-tokens/blob/master/ERC20_token_issue_list.md#a20-re-approve
But personally I don't think it's a great burden to do what @martonp described.
There was a problem hiding this comment.
If we do an automated thing, we should just have the allowance be unlimited
That is the default behavior (default set below; see also impl. in #1381).
Changing allowances is tricky business
I'm not seeing how turning the allowance over for users to manually edit fixes anything. If it's tricky because of the linked issues (double-dip race), manual entering adjustments doesn't fix that. If it's tricky because the operations need to be done carefully, then we definitely don't want the users doing it.
But personally I don't think it's a great burden to do what @martonp described.
Requiring users to manually type numbers into inputs to adjust allowances seems to me like a recipe for bad UX, but we can probably expose some options or order-time options to gain more control. One thing I think is inevitable is having a custom Details interface{} field in the asset.Balance that the Wallet can populate. Tokens could list the current allowance there. I think that's one part necessary to a sensible UI. Then maybe an order-time option to pre-approve additional lots when placing an order. Is that sounding reasonable?
I'll keep thinking about ways to make it work.
There was a problem hiding this comment.
That is the default behavior (default set below; see also impl. in #1381).
Problem solved imo.
but we can probably expose some options or order-time options to gain more control.
I don't see these allowance adjustments being possible at order time since it involves multiple transactions and since it affects all ongoing swaps. If we ever do want a manually set (lower than infinity) allowance, a well executed wallet function is the way to go. I don't think we need to bother with this any time soon though.
One thing I think is inevitable is having a custom Details interface{} field in the asset.Balance that the Wallet can populate.
That'll probably be needed for dcr mixing wallets if we want other balance categories too.
There was a problem hiding this comment.
I'm sure you know this, but just so everyone's on the same page, the allowance is consumed as transferFrom is called.
So they can't just "set" it, they have to either manually add it 1) for every order (bad UX), or 2) less frequently but in values larger than necessary (defeats the purpose of a limited allowance). That's how I came to do unlimited by default, otherwise 1 but adjust automatically at order time.
There was a problem hiding this comment.
As of right now, I'm planning to remove the option. We can revisit it at a later time.
There was a problem hiding this comment.
I don't think setting it less frequently in larger values defeats the purpose.. if you have 50k usdc in the wallet and you only allow 5k it's still way safer. If setting the allowance was free and quick, then you could adjust at order at order time, but in reality it costs $10 each time and you'd have to wait a few confirmations before the trade could be booked. But yeah I agree, let's just do unlimited first.
There was a problem hiding this comment.
I would just limit the amount of the token kept in this account. I would hope users would NOT treat the Dex eth account like long term storage of more than they might trade.
Then again if you are just buying the token, not selling, maybe you'd want 0 allowance
There was a problem hiding this comment.
Yeah they should be able to buy the token without approval, no point in wasting the gas fees if someone only wants to buy, but if they go to sell, they should not be able to until they do an approval. There could be a button on the market page as well allowing them to do an unlimited approval.
dex: Token and Gases are used to specify token info in higher-level packages, where lgpl builds are not suitable. Add a test token ID. client/asset: The Token type embeds dex.Token and adds a WalletDefinition. This type is used to register tokens with the package-level RegisterToken method. client/asset/eth: The test token is registered with it's WalletDefinition. The definition has 1 ConfigOption, limitAllowance, that allows the user to keep a limited allowance for the swap contract, but with extra transactions = more fees.
buck54321
force-pushed
the
erc20-dex-asset
branch
from
4eed29a to
1b488b9
Compare
Taken from #1381