client/asset/eth: finish client wallet tokens #1399

buck54321 · 2022-01-11T22:26:49Z

I've peeled a few other PRs off of this one, but I think I'll submit the rest as one. Lots of tests written or adapted and all are passing. Ready for a look.

Introduce tokenContractor interface to handle operations on the token
contract and token-bound swap contract.

Separate ExchangeWallet into baseWallet and AssetWallet.

Move contractor handling to AssetWallet. This simplifies the ethFetcher
and is a better division of labor, making future implementations of
EVM-family assets easier.

Implement detailed calculations of swap and approve gas, issuing
warnings when a configuration mismatch is detected.

dex/networks/eth,dex:
Simplify dex.Token and create dexeth.Token type to add gas and address
info. Move gas info to version-specific token data.

martonp

Initial Pass, looks good. I like the base/asset wallets and the contractors being moved out of the nodeclient. I think the biggest issue I found is regarding the unit conversion when initiating swaps.

client/asset/estimation.go

client/asset/eth/contractor.go

martonp · 2022-01-15T15:01:48Z

client/asset/eth/eth.go

+				return nil, nil, fmt.Errorf("parent balance %d doesn't cover contract approval (%d) and tx fees (%d)",
+					ethBal.Available, approveGas*cfg.MaxFeeRate, ethToLock)
+			}
+			if _, err := w.approveToken(unlimitedAllowance, cfg.MaxFeeRate, cfg.Version); err != nil {


I haven't read the whole PR yet, but I'm assuming that on the first sell trade of the token, this is where the approval would happen. If this is the case then there would need to be additional handling on the server, because this trade cannot yet be booked. Having the unlimited approval as a manual step completely avoids this issue. I know you think this is a bad user experience, but anyone who has used an erc20 token is completely familiar with this step. It's just an extra click before you sell the token.

Also, I think these checks of the allowance going below 2^256/2 are not necessary. Other than maybe some absolute ridiculous token, there will be no tokens anywhere near 2^256 max supply, and even for that ridiculous token, the current user would have had to initiate swaps with the entire supply of the token before their allowance would run out, so I don't think we need to worry about that.

there would need to be additional handling on the server, because this trade cannot yet be booked

Can you expand on this? Doesn't the nonce order guarantee that the approval will be processed first.

From what I can tell, we're not checking the client's allowance server-side, and I didn't think we had any plans to do that. I was just going to relegate it to the client as part of configuration. Nothing we could do can prevent a client from sending an approve(contractAddr, 0) through after we accept their order anyway.
The cost of approval is next to nothing, compartively, and correctly reported in the pre-order estimates.

If we did want to check allowance server-side, we would have to add latency handling though, which is what I think you were getting at.

To add manual approval, we would need to add a new UI dialog, new endpoint in webserver, new exported method from *Core, new exported method from eth.ExchangeWallet, and a new method or specialty interface in interface.go. And then we need to maintain all of that forever. And that's so we can force a user to click a button that they have no choice but to click in order to use the wallet that they just created. We also have to explain why we are forcing them to separately click this button to use the wallet. Why would they click "Create" if they were then going to say no to configuring the wallet for use? It makes for a bad UX, imo. Approval is not optional. It's part of trading with this asset. We don't need to pass the burden on to the user.

I think the only case approval is really not needed is if you're just buying the token, not selling it.

Still, I completely support keeping approval an automated task for simplicity and UX reasons.

Hm I'm probably biased because all the other apps I've used have had approval as a manual step. I think you're right, we don't need to do any validation on the server side.

I think the ideal way would be the approval happens automatically when the user is doing their first buy order. In that case this code that does the approval should be in ReserveN instead of FundOrder.

Some of the token primitives used in decred#1399, offered separately here to ease the integration of that work. dex: Add a simple Token type, outlining the parent-child link and the UnitInfo. Move other token details to dex/networks/eth dex/networks/erc20: Rename TOKEN_ADDRESS method to token_address so that the Go method will be TokenAddress instead of TOKENADDRESS. Add tx data parsers for transfer and transferFrom calls. dex/networks/eth: Export calldata parsing machinery. Add token details and Gases that used to be in /dex.

Some of the token primitives used in decred#1399, offered separately here to ease the integration of that work. dex: Add a simple Token type, outlining the parent-child link and the UnitInfo. Move other token details to dex/networks/eth dex/networks/erc20: Rename TOKEN_ADDRESS method to token_address so that the Go method will be TokenAddress instead of TOKENADDRESS. Add tx data parsers for transfer and transferFrom calls. dex/networks/eth: Export calldata parsing machinery. Add token details and Gases that used to be in /dex. Add methods for handling tokens with decimals != 18.

chappjc

Looking pretty slick so far. Still going through it.

client/asset/interface.go

client/asset/eth/contractor.go

client/asset/eth/eth.go

chappjc · 2022-04-27T16:54:40Z

client/asset/eth/eth.go

-// client app communicates with the Ethereum blockchain and wallet. ExchangeWallet
-// satisfies the dex.Wallet interface.
-type ExchangeWallet struct {
+type baseWallet struct {


I'm not fully grasping the purpose of a separate baseWallet. It does not seem to be embeded or used by anything other than AssetWallet. You hinted at other EVM-based assets, so is it around for possible other chain impls? If so, should AssetWallet be ETHWallet, and what about methods like func (*baseWallet) Info() *asset.WalletInfo that appear to be strictly Ethereum?
No objections, just trying to see your end game. Like if we did polygon, roughly how might you organize things?

other EVM-based assets

Still these would not use this baseWallet as they would be in a different package. Srry obvious comment.

There is only one baseWallet shared by all AssetWallets.

dcrdex/client/asset/eth/eth.go

Line 512 in 64b5c1e

baseWallet: w.baseWallet,

I am going to look at ETHWallet and TokenWallet structs as per @JoeGruffins request though.

There is only one baseWallet shared by all AssetWallets.

Yah, this was really what I was commenting about. Made me uncertain on why there where two separate types. Will see what happens with the next iteration.

It's because there's only one instance of baseWallet that holds the reference to the node. Then there are multiple instances of AssetWallet each of which hold a reference to the one baseWallet. It's not that each AssetWallet is holding a reference to its own baseWallet.

client/asset/eth/eth.go

chappjc · 2022-04-27T17:08:26Z

client/asset/eth/eth.go

+	// Not gonna worry about accuracy for nSwap. It's never used where it
+	// matters, just for estimates.
+	nSwap = oneSwap + uint64(n-1)*g.SwapAdd


What do you mean by worry about accuracy? As in just using the hard coded and padded estimates for this contract version?
There doesn't seem to be a possibility of overflowing uint64 here with values in gwei. If that's a concern that would be an issue, but I don't see that.

JoeGruffins

Looks good. I hope that you can add a TokenWallet similar to #1404 because I think that would clean up these methods a lot and make them easier to reason about without all the w.assetID == BipID checks.

client/asset/eth/contractor.go

client/asset/eth/eth.go

JoeGruffins · 2022-04-28T05:32:26Z

client/asset/eth/eth.go

-// client app communicates with the Ethereum blockchain and wallet. ExchangeWallet
-// satisfies the dex.Wallet interface.
-type ExchangeWallet struct {
+type baseWallet struct {


other EVM-based assets

Still these would not use this baseWallet as they would be in a different package. Srry obvious comment.

client/asset/eth/eth.go

JoeGruffins · 2022-04-28T06:14:10Z

client/asset/eth/eth.go

-	eth.lockedFunds.mtx.Lock()
-	defer eth.lockedFunds.mtx.Unlock()
-	err := eth.lockFunds(initiationFunds, initiationReserve)


Still need to hold the lock to do both locking and unlocking I think.

client/asset/eth/eth.go

Introduce tokenContractor interface to handle operations on the token contract and token-bound swap contract. Separate ExchangeWallet into baseWallet and AssetWallet. Move contractor handling to AssetWallet. This simplifies the ethFetcher and is a better division of labor, making future implementations of EVM-family assets easier. Implement detailed gas cost resolution for swaps and redeems, issuing warnings when a configuration mismatch is detected. dex/networks/eth,dex: Simplify dex.Token and create dexeth.Token type to add gas and address info. Move gas info to version-specific token data. I plan on moving the re-assigned *AssetWallet methods from nodeclient.go to eth.go, but leaving them now for reviewability.

martonp

Started reviewing, will continue later.

client/asset/eth/contractor.go

client/asset/eth/eth.go

The expected return value is *asset.WalletInfo, but the only fields used anywhere by us are UnitInfo, Version, and Name. We should use a different type, but adapting to the current interface for now.

client/asset/eth/eth.go

client/asset/interface.go

chappjc

I really like the latest change of splitting ETHWallet and TokenWallet. In general it makes things much easier to follow and less error prone, even if at the expense of a little extra code and boiler plate. Worth it.

JoeGruffins

Just a few more comments.

The eth harness tests are failing with participant newV0TokenContractor error: error reading bound token address: no contract code at given address

JoeGruffins · 2022-04-29T04:23:47Z

client/asset/eth/eth.go

+	}
+
+	aw := &AssetWallet{
+		baseWallet:         w.baseWallet,


Similar to the server nodes, I think separate sub loggers with names identifying the token would be better for debugging.

JoeGruffins · 2022-04-29T04:57:48Z

client/asset/eth/eth.go

+
+// CreateTokenWallet "creates" a wallet for a token. There is really nothing
+// to do, except check that the token exists.
+func (eth *baseWallet) CreateTokenWallet(tokenID uint32, _ map[string]string) error {


nit: eth unused so no need to declare.

JoeGruffins · 2022-04-29T05:02:46Z

client/asset/eth/eth.go

+	}
+
+	aw := &AssetWallet{
+		baseWallet:         w.baseWallet,


Oh, I finally understand why you have both the baseWallet type and the AssetWallet type. The base wallet is shared without changes like this. The other AssetWallet fields are recreated for every token asset. Sorry that took a while for me to get.

I was being seriously dense about this too.

JoeGruffins · 2022-04-29T05:14:41Z

client/asset/eth/eth.go

-func (eth *ExchangeWallet) createFundingCoin(amount uint64) *fundingCoin {
+func (eth *baseWallet) createFundingCoin(amount uint64) *fundingCoin {
 	return createFundingCoin(eth.addr, amount)
 }

-// decodeFundingCoinID decodes a coin id into a coin object. This function ensures
-// that the id contains an encoded fundingCoinID whose address is the same as
-// the one managed by this wallet.
-func (eth *ExchangeWallet) decodeFundingCoinID(id []byte) (*fundingCoin, error) {
-	fc, err := decodeFundingCoin(id)
+func (eth *baseWallet) createTokenFundingCoin(amount, fees uint64) *tokenFundingCoin {
+	return createTokenFundingCoin(eth.addr, amount, fees)
+}


These two may make more sense for their respective types rather than *baseWallet. Can both be the same name then too, I think.

JoeGruffins · 2022-04-29T05:20:54Z

client/asset/eth/eth.go

+// allowanceGasRequired estimates the gas that is required to issue an approval
+// for a token. If the dexRedeemCfg is not for a fee-family erc20 asset, no
+// error is returned and the return value will be zero.
+func (w *AssetWallet) allowanceGasRequired(dexRedeemCfg *dex.Asset) (uint64, error) {
+	if dexRedeemCfg.ID == BipID {
+		return 0, nil
 	}


nit: From the description it looks like this should be off the *TokenWallet. I realize why it's annoying to change for this one though, so count this as a nit.

JoeGruffins · 2022-04-29T07:36:16Z

client/asset/eth/eth.go

+	err := w.lockFunds(reserve, refundReserve)

 	return reserve, err


Should return 0 on error. I realize this is previous behavior.

JoeGruffins · 2022-04-29T07:41:48Z

client/asset/eth/eth.go

+// TODO: Lock, Unlock, and Locked should probably be part of an optional
+// asset.Authenticator interface that isn't implemented by token wallets.
+// This is easy to accomplish here, but would require substantial updates to
+// client/core.
+// The addition of an ETHWallet type that implements asset.Authenticator would
+// also facilitate the appropriate compartmentalization of our asset.TokenMaster
+// methods, which token wallets also won't need.
+


Hmm. If you lock any of the tokens all the "wallets" get locked. This is probably a big problem we need to solve before going live. We can't lock any of them with active trades in any others, and I'm pretty sure core doesn't check for that at the moment because would be different assets.

I'm not sure I see much reason to allow locking an eth account at all. As far as I'm concerned, they can be unlocked permanently when user logs in and the native eth wallet is loaded. The app pass is still required to do various things, and always for withrdaw.

Alternatively, for a gentle solution to this TODO comment, how about overriding the Lock method with a dummy func (*TokenWallet).Lock() error { return nil }.

JoeGruffins · 2022-04-29T07:51:57Z

client/asset/eth/eth.go

-	if !found {
-		return fmt.Errorf("no version %d contractor", ver)
+func (w *AssetWallet) balanceWithTxPool() (*Balance, error) {
+	isETH := w.assetID == BipID


nit: Still checking against the BipID.

There was too much shared here. I didn't want to duplicate all of it.

JoeGruffins · 2022-04-29T08:08:52Z

client/asset/eth/eth.go

+// withTokenContractor runs the provided function with the tokenContractor.
+func (w *AssetWallet) withTokenContractor(assetID, ver uint32, f func(tokenContractor) error) error {
+	return w.withContractor(ver, func(c contractor) error {
+		tc, is := c.(tokenContractor)
+		if !is {
+			return fmt.Errorf("contractor for %d %T is not a tokenContractor", assetID, c)
+		}
+		return f(tc)
+	})
+}
+
+// estimateApproveGas estimates the gas required for a transaction approving a
+// spender for an ERC20 contract.
+func (w *AssetWallet) estimateApproveGas(newGas *big.Int) (gas uint64, err error) {
+	return gas, w.withTokenContractor(w.assetID, contractVersionNewest, func(c tokenContractor) error {
+		gas, err = c.estimateApproveGas(w.ctx, newGas)
+		return err
+	})
+}
+
+// estimateTransferGas estimates the gas needed for a token transfer call to an
+// ERC20 contract.
+func (w *AssetWallet) estimateTransferGas(val uint64) (gas uint64, err error) {


These three look like they should be off of TokenWallet.

Damn. I can't do this because of some other shared code. Maybe it would be worth splitting more methods up, but I'm not feeling it. Can follow up.

JoeGruffins · 2022-04-29T08:15:02Z

client/asset/eth/eth.go

+// getGasEstimate is used to get a gas table for an asset's contract(s). The
+// provided gases, g, should be generous estimates of what the gas might be.
+// Errors are thrown if the provided estimates are too small by more than a
+// factor of 2. The account should already have a trading balance of at least
+// maxSwaps gwei (token or eth), and sufficient eth balance to cover the
+// requisite tx fees.
+func GetGasEstimates(ctx context.Context, cl *nodeClient, c contractor, maxSwaps int, g *dexeth.Gases, waitForMined func()) error {


Comment is unexported, but this is exported. Will this be part of another tool? Does it belong in testing? Uses waitForMined() which is part of the harness tests. Not sure if you can use functions from test files in the build.

waitForMined is passed in as an argument for now. This will be used to create the gas tables once a contract is launched on a network. I haven't written a utility for it yet, but I'm planning on it.

chappjc · 2022-04-30T13:55:19Z

Thanks for the hard work guys. I think we can move forward. @JoeGruffins If there's anything else that pops out or looks to have slipped by, there's clearly gonna be followup.

buck54321 force-pushed the erc20-client-eth branch from 028137d to def2b4e Compare January 11, 2022 23:29

buck54321 mentioned this pull request Jan 12, 2022

client/core,eth: Token Wallet #1369

Closed

buck54321 force-pushed the erc20-client-eth branch from def2b4e to 79d8cd0 Compare January 15, 2022 12:43

martonp reviewed Jan 15, 2022

View reviewed changes

buck54321 force-pushed the erc20-client-eth branch 2 times, most recently from a99574a to 00fd409 Compare March 9, 2022 01:55

buck54321 mentioned this pull request Mar 9, 2022

dex/networks/{eth,erc20}: common token stuff #1509

Merged

buck54321 force-pushed the erc20-client-eth branch from 00fd409 to cc9793a Compare March 15, 2022 23:08

buck54321 mentioned this pull request Mar 16, 2022

client/asset/eth: token prefactor #1523

Merged

buck54321 force-pushed the erc20-client-eth branch 3 times, most recently from a26b27d to 495ca4f Compare April 13, 2022 12:38

buck54321 force-pushed the erc20-client-eth branch 3 times, most recently from f38bcc4 to 64b5c1e Compare April 24, 2022 16:04

buck54321 marked this pull request as ready for review April 24, 2022 16:07

chappjc requested review from JoeGruffins and chappjc April 25, 2022 19:10

chappjc mentioned this pull request Apr 25, 2022

client/eth: Get receipt before checking status. #1599

Merged

chappjc requested changes Apr 27, 2022

View reviewed changes

JoeGruffins reviewed Apr 28, 2022

View reviewed changes

martonp reviewed Apr 28, 2022

View reviewed changes

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

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

separate wallet types and other review followup

e5c5d32

buck54321 force-pushed the erc20-client-eth branch from 64b5c1e to e5c5d32 Compare April 28, 2022 14:23

partial resolution of (asset.Wallet).Info.

82f7984

The expected return value is *asset.WalletInfo, but the only fields used anywhere by us are UnitInfo, Version, and Name. We should use a different type, but adapting to the current interface for now.

chappjc reviewed Apr 28, 2022

View reviewed changes

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

chappjc reviewed Apr 28, 2022

View reviewed changes

client/asset/interface.go Outdated Show resolved Hide resolved

chappjc approved these changes Apr 28, 2022

View reviewed changes

more review followup

3521f65

JoeGruffins reviewed Apr 29, 2022

View reviewed changes

martonp approved these changes Apr 30, 2022

View reviewed changes

fixes per review

4b66616

chappjc merged commit 2035639 into decred:master Apr 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client/asset/eth: finish client wallet tokens #1399

client/asset/eth: finish client wallet tokens #1399

buck54321 commented Jan 11, 2022 •

edited

martonp left a comment

martonp Jan 15, 2022

buck54321 Jan 27, 2022

chappjc Jan 27, 2022 •

edited

martonp Jan 27, 2022

chappjc left a comment

chappjc Apr 27, 2022

JoeGruffins Apr 28, 2022

buck54321 Apr 28, 2022

chappjc Apr 28, 2022

martonp Apr 30, 2022 •

edited

chappjc Apr 27, 2022 •

edited

JoeGruffins left a comment

JoeGruffins Apr 28, 2022

JoeGruffins Apr 28, 2022

martonp left a comment

chappjc left a comment

JoeGruffins left a comment

JoeGruffins Apr 29, 2022

JoeGruffins Apr 29, 2022

JoeGruffins Apr 29, 2022

chappjc Apr 30, 2022

JoeGruffins Apr 29, 2022 •

edited

JoeGruffins Apr 29, 2022 •

edited

JoeGruffins Apr 29, 2022

JoeGruffins Apr 29, 2022

chappjc Apr 29, 2022

JoeGruffins Apr 29, 2022

buck54321 Apr 30, 2022

JoeGruffins Apr 29, 2022

buck54321 Apr 30, 2022

JoeGruffins Apr 29, 2022

buck54321 Apr 30, 2022

chappjc commented Apr 30, 2022

		err := w.lockFunds(reserve, refundReserve)

		return reserve, err

client/asset/eth: finish client wallet tokens #1399

client/asset/eth: finish client wallet tokens #1399

Conversation

buck54321 commented Jan 11, 2022 • edited

martonp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Jan 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martonp Apr 30, 2022 • edited

Choose a reason for hiding this comment

chappjc Apr 27, 2022 • edited

Choose a reason for hiding this comment

JoeGruffins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martonp left a comment

Choose a reason for hiding this comment

chappjc left a comment

Choose a reason for hiding this comment

JoeGruffins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeGruffins Apr 29, 2022 • edited

Choose a reason for hiding this comment

JoeGruffins Apr 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc commented Apr 30, 2022

buck54321 commented Jan 11, 2022 •

edited

chappjc Jan 27, 2022 •

edited

martonp Apr 30, 2022 •

edited

chappjc Apr 27, 2022 •

edited

JoeGruffins Apr 29, 2022 •

edited

JoeGruffins Apr 29, 2022 •

edited