Skip to content
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

multi: enable token trading #1622

Merged
merged 6 commits into from
Aug 11, 2022
Merged

multi: enable token trading #1622

merged 6 commits into from
Aug 11, 2022

Conversation

buck54321
Copy link
Member

@buck54321 buck54321 commented May 19, 2022

client/asset:
Add allowable networks to RegisterToken. The client can then use
the new package function SetNetwork during initialization to filter out
incompatible tokens.

client/asset/eth:
Change OpenTokenWallet to use a single struct argument. Add PeersChange
function and notify token wallets of peer changes.
Fix bugs in swap fee estimation to deal with transfer approval.

client/core:
Plumb in token wallet creation and opening.
Add TokenInfo field to SupportedAsset, populated instead of the
Info field for token wallets. Should consider changing the name
of the Info field to better illustrate the purpose and to break
Go consumer's API to force adaptation to the new semantics.

client/webserver:
Add token to live tests.

ui:
Implement new wallet creation flow for token wallets that includes
a step to sync the parent. Adapt to new SupportedAsset structure.
Add "token-aware symbols", which are symbols that, for tokens, have
an additional <sup> element denoting the parent asset.

testing:
Loadbot and core simnet tests are updated for dextt.eth.

Copy link
Member

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loadbot was working fine before, but now seeing 2022-06-27 16:09:45.067 [CRT] MANTLE:CMPD:STACKER:0: failed to send funds to alpha: invalid hex address "\"0x18d65fb8d60c1199bb1ad381be47aa692b482605\""

It seems getCodeAt is failing for the eth simnet tests.

I think that the core method assetHasActiveOrders now needs more logic if the asset is a asset.TokenMaster we need to also check active orders for all assets this is a parent wallet of.

Seems to be working very well.

One UI issue, <invalid coin>:
image

@@ -10,10 +10,17 @@ import (
"decred.org/dcrdex/dex"
)

// nettedToken is a Token with it's available networks. Saving the valid
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's -> its

Comment on lines 243 to 256
p := n.leth.ApiBackend.SyncProgress()
if p.HighestBlock == 0 && p.CurrentBlock > 0 {
// If we shutdown and restart without going out of sync with peers,
// the HighestBlock will never be set.
// I guess just assume that if we have peers and HighestBlock is zero,
// that we're synced.
// TODO: Check for a race here. Can we pull a peer before we know its
// best block?
if n.p2pSrv.PeerCount() > 0 {
p.HighestBlock = p.CurrentBlock
}
}

return p
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's no need to change this now with #1630

Is it still broken?

// Start a goroutine to wait until the parent wallet is synced, and then
// begin creation of the token wallet.
c.wg.Add(1)
c.pendingCreateWallet.Store(&assetID)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we can only ever have one pending wallet at a time? Does this need logic for multiple create wallet attempts with different tokens? Because parent creation should be synchronous, it wouldn't make it here with only one parent assetID (eth). But if there were more?

It looks like if another asset were pending, it would just overwrite here.

If the same parent, the second token wallet would not wait for the parent to sync, although creation would be synchronous.

It also looks like if _, found := c.wallet(ID) could return not found for two calls close together, and create either the token or parent multiple times. I guess this was possible before though.

@martonp
Copy link
Contributor

martonp commented Jul 3, 2022

Screen Shot 2022-07-03 at 12 23 38 PM

"Balances" is slipping out of the box.

@martonp
Copy link
Contributor

martonp commented Jul 3, 2022

Screen Shot 2022-07-03 at 3 28 53 PM

This is probably a separate issue, but it doesn't make sense to show these reconfigure sections for wallets that have nothing to reconfigure.

@@ -530,7 +534,7 @@ export default class WalletsPage extends BasePage {
this.lastFormAsset = assetID
const wallet = app().walletMap[assetID]
if (!wallet) {
app().notify(ntfn.make('Cannot send/withdraw.', `No wallet found for ${asset.info.name}`, ntfn.ERROR))
app().notify(ntfn.make('Cannot send/withdraw.', `No wallet found for ${asset.name}`, ntfn.ERROR))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Below on line 558 - asset.info.unitinfo is causing an error. It would be better if the info and token fields were optional and the ts compiler forced you to check if it is defined.

// SetNetwork will filter registered assets for those available on the specified
// network. SetNetwork need only be called once during initialization.
func SetNetwork(net dex.Network) {
nextasset:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this nextasset needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I want to continue here and not at the for _, allowedNet := range nt.nets { loop.

@@ -621,8 +622,8 @@ func (w *assetWallet) lockFunds(amt uint64, t fundReserveType) error {
}

if balance.Available < amt {
return fmt.Errorf("attempting to lock more for %s than is currently available. %d > %d",
t, amt, balance.Available)
return fmt.Errorf("attempting to lock more %s for %s than is currently available. %d > %d",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a preexisting issue, but since these error messages show up on the UI, they should use the proper units.

}
},
DataDir: c.assetDataDirectory(assetID),
token := asset.TokenInfo(assetID)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NBD, but I would put this right before the if token == nil check.

@@ -238,4 +239,6 @@ var EnUS = map[string]string{
"successful_cert_update": "Successfully updated certificate!",
"update dex host": "Update DEX Host",
"copied": "Copied!",
"Synchronizing": "Synchronizing",
"wallet_wait_synced": "wallet will be created after sync",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wallet will be created after parent wallet syncs?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meh. It says "Syncing Ethereum" right above this message.

@@ -76,6 +121,22 @@ export class NewWalletForm {
else Doc.show(...this.pwHiders)
}

async createWallet (assetID: number, walletType: string, pw: string, parentForm?: WalletConfig | null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need the | null? Isn't the ? enough?

Token: token,
Name: token.Name,
UnitInfo: token.UnitInfo,
WalletCreationPending: tokenID == creatingWalletID,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If two token wallets are being created concurrently the one that started first would have this set to false.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was resolved in 9ebb3da, I think.

Copy link
Member

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See this often in logs:

2022-07-19 14:31:08.166 [ERR] CORE[eth][DEXTT.ETH]: (60000) error estimating swap gas: gas required exceeds allowance (11673760)
2022-07-19 14:31:08.166 [ERR] WEB: max order estimation error: dextt.eth wallet MaxOrder error: error getting swap gas estimate: gas required exceeds allowance (11673760)

And this once:

2022-07-19 14:24:49.438 [ERR] CORE: retiring order 9ac2316cdb9559eafd9d4c3c63ef87c815a6b45b1a745e2409733e51929c0080 with 100000000 > 0 refund funds locked

But looks good to me!

dex/networks/eth/tokens.go Outdated Show resolved Hide resolved
_ "decred.org/dcrdex/server/asset/eth" // register eth asset
)

func init() {
dexeth.MaybeReadSimnetAddrs()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't feel good to be doing this with production/mainnet code, but MaybeReadSimnetAddrs clearly only touches the dex.Simnet map entries so I guess it's alright.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel similarly. Better solution desired.

for _, in := range inits {
sum += in.Value
}
if bc.value < sum {
Copy link
Member

@chappjc chappjc Jul 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a token where bc.value is ETH and inits[i].Value is the token unit this check doesn't work so you skip it, but is it OK to just let the inits have any values? Would there ideally be balance check on the from account's token x balance? Does that get caught somewhere else or just failed swap (and penalization)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be filtered by the if eth.assetID == BipID { above. Should probably rename the receiver for AssetBackend.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, that's what I mean by "this check doesn't work so you skip it". Still uncertain if an analogous check in the proper units is needed or happens somewhere else already.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sent me down a rabbit-hole actually. We were still using gwei in the dexeth.Initiation struct, which was shared between tokens and eth, so needed consideration of the contract's decimals. Switched it to a *big.Int in evm units, and that echoed through a few places.

That would have been an issue for e.g. usdc.

client/asset/driver.go Outdated Show resolved Hide resolved
@@ -1475,6 +1502,21 @@ func (c *Core) storeDepositAddress(wdbID []byte, addr string) error {

func (c *Core) connectAndUpdateWallet(w *xcWallet) error {
assetID := w.AssetID

token := asset.TokenInfo(w.AssetID)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assetID is declared above, might as well use it here and a few places in this new code

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still using w.AssetID in two spots right below this.

client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Show resolved Hide resolved
@buck54321
Copy link
Member Author

buck54321 commented Jul 21, 2022

See this often in logs:

2022-07-19 14:31:08.166 [ERR] CORE[eth][DEXTT.ETH]: (60000) error estimating swap gas: gas required exceeds allowance (11673760)
2022-07-19 14:31:08.166 [ERR] WEB: max order estimation error: dextt.eth wallet MaxOrder error: error getting swap gas estimate: gas required exceeds allowance (11673760)

Yeah, isn't that curious? That happens when running estimateGas. I've looked into it, but it's confusing. It seems like geth thinks we're hitting some kind of limit on the total gas spent in a block or something. I'm almost certain it's a simnet thing, but we should figure it out.

I actually thought this was happening sometimes for ETH too, but maybe I was mistaken. Will test more.

@buck54321
Copy link
Member Author

I'm still looking into insufficient balance errors for the fee asset = eth from server in loadbot.

@chappjc
Copy link
Member

chappjc commented Jul 22, 2022

Heaps of conflicts with #1600 merged, but I squashed and rebased locally without trouble, so looks mostly trivial. A bunch of .info for the most part.

Copy link
Contributor

@martonp martonp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working well, I was unable to make anything go wrong. Just need to fix that balances text on the market page.

// Test token
registerToken(testTokenID, "A token wallet for the DEX test token. Used for testing DEX software.")
registerToken(testTokenID, "A token wallet for the DEX test token. Used for testing DEX software.", dex.Simnet)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of hardcoding dex.Simnet over here, you could check which networks are defined in the Tokens map in dex/networks/eth/token.go. nbd though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately not, because we won't have called MaybeReadSimnetAddrs yet. I'm open to other solutions though.

@@ -645,8 +646,8 @@ func (w *assetWallet) lockFunds(amt uint64, t fundReserveType) error {
}

if balance.Available < amt {
return fmt.Errorf("attempting to lock more for %s than is currently available. %d > %d",
t, amt, balance.Available)
return fmt.Errorf("attempting to lock more %s for %s than is currently available. %d > %d gwei",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It won't be gwei if it's a token.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an issue in many other places as well, the error messages are a little confusing when they show up on the UI. I can do another PR for this after this one goes in.

if err != nil {
return err
}
if err = c.setWalletCreationPending(assetID); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it cause an issue if someone quickly clicks to create two separate token wallets, and it will simultaneously start creating the parent ETH wallet twice?

// Create the parent synchronously.
parentWallet, err := c.createWalletOrToken(crypter, walletPW, form.ParentForm)
if err != nil {
c.setWalletCreationComplete(assetID)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be a defer after you call setWalletCreationPending . It's good now, but might be easy to forget if someone puts some additional code here for some reason.

@@ -467,3 +467,9 @@ div.border1 {
.preline {
white-space: pre-line;
}

Copy link
Contributor

@martonp martonp Aug 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In market.scss there in the .market-bal-lbl section, there is a top: -11px which is causing the Balances: text to leave the box. I see it on both firefox and chrome. Maybe this was needed before, but now it's not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was on purpose, but needs updating. I'll be doing some work ont the markets page style soon.

@chappjc
Copy link
Member

chappjc commented Aug 8, 2022

In client/core/trade.go, we should use the Receipt's stringer:

diff --git a/client/core/trade.go b/client/core/trade.go
index df0352f0..fbb86ffc 100644
--- a/client/core/trade.go
+++ b/client/core/trade.go
@@ -2074,7 +2074,7 @@ func (c *Core) swapMatchGroup(t *trackedTrade, matches []*matchTracker, errs *er
                match := matches[i]
                coin := receipt.Coin()
                c.log.Infof("Contract coin %v (%s), value = %d, refundable at %v (script = %v), match = %v",
-                       coin, t.wallets.fromAsset.Symbol, coin.Value(), receipt.Expiration(), receipt.Contract(), match)
+                       coin, t.wallets.fromAsset.Symbol, coin.Value(), receipt.Expiration(), receipt.String(), match)
                if secret := match.MetaData.Proof.Secret; len(secret) > 0 {
                        c.log.Tracef("Contract coin %v secret = %x", coin, secret)
                }

With .Contract() it's the undecoded "contract data" bytes:

2022-08-08 15:54:45.014 [INF] CORE: Contract coin 0x1c1009fea9a1c04f44c7dc6d4bf8db9ce1c2f6d727d836817e7ed1f45449fb6c (dextt.eth), value = 666600000, refundable at 2022-08-08 16:00:45 -0500 CDT (script = 000000000290c8950d9094f0c881ba16850d2c50bb6487c83e6d4336f0f530a995a4e75a), match = 58d2ca2823638d87d85163df46795b91ac759c0c0aedc79ff090b668d9d2b503

It's also not a "script", but that's just a holdover from utxo script obviously.

With the Stringer, it'll give something meaningful even if it's redundant with the Broadcasted transaction with X swap contracts for order log just prior:

2022-08-08 16:12:30.016 [INF] CORE: Contract coin 0xed2267dfb8e4b79fb518bcd47c15ce691fff3e849246347a764e4dca11474fc9 (dextt.eth), value = 46000000000, refundable at 2022-08-08 16:18:30 -0500 CDT (script = { tx hash: ed2267dfb8e4b79fb518bcd47c15ce691fff3e849246347a764e4dca11474fc9, contract address: 0x6b4368d3E41a60e20FF8539C843B3CDB38C8A507, secret hash: 9efdf6fc87bfdf41c91cffc1115076c4017538f075026c0beddb6ae2aa7f884f }), match = a83b886d29adc0f200cf40271a7b722c88b19f235011227418aaed5d5b3554dc

Maybe also we change contract = to data = or contract data =?

@chappjc
Copy link
Member

chappjc commented Aug 8, 2022

See this often in logs:

2022-07-19 14:31:08.166 [ERR] CORE[eth][DEXTT.ETH]: (60000) error estimating swap gas: gas required exceeds allowance (11673760)
2022-07-19 14:31:08.166 [ERR] WEB: max order estimation error: dextt.eth wallet MaxOrder error: error getting swap gas estimate: gas required exceeds allowance (11673760)

Yeah, isn't that curious? That happens when running estimateGas. I've looked into it, but it's confusing. It seems like geth thinks we're hitting some kind of limit on the total gas spent in a block or something. I'm almost certain it's a simnet thing, but we should figure it out.

I actually thought this was happening sometimes for ETH too, but maybe I was mistaken. Will test more.

On mainnet, block gas limit has been increasing and is currently ~30 million in the post-london era, and geth says that --dev.gaslimit is default of 11.5 million as the starting gas limit, so if this estimate gas call wants more than 11.9 million, a huge portion of a real block's gas limit. That suggest to me it's for a crazy large number of lots making an unrealistically gas hungry txn.

This seems to be supported by the message if you don't have a lot of fee asset (try an exchange rate like 0.02 dextt/dcr):

2022-08-08 16:57:27.108 [DBG] CORE[eth][DEXTT.ETH]: Skipping live swap gas because contract is not approved for transferFrom
2022-08-08 16:57:27.108 [INF] CORE[eth][DEXTT.ETH]: MaxOrder reducing lots because of low fee reserves: 16666 -> 1379
2022-08-08 16:57:27.108 [DBG] CORE[eth][DEXTT.ETH]: Skipping live swap gas because contract is not approved for transferFrom

But I must be confused because the following gasses indicate we'd use and entire mainnet block with about a swap tx with ~265 inits in the batch. Even 10 lots, which is reasonable, would mean ~%4 of an entire block's gas limit? Can that be right?

var v0Gases = &Gases{
	Swap:      135000,
	SwapAdd:   113000,
	Redeem:    63000,
	RedeemAdd: 32000,
	Refund:    43000,
}

debugging:

diff --git a/client/asset/eth/eth.go b/client/asset/eth/eth.go
index f853fb38..eb1fc0e4 100644
--- a/client/asset/eth/eth.go
+++ b/client/asset/eth/eth.go
@@ -1047,6 +1047,8 @@ func (w *assetWallet) swapGas(n int, dexCfg *dex.Asset) (oneSwap, nSwap uint64,
                return oneSwap, nSwap, false, nil
        }
 
+       w.log.Infof("swapgas: %d / %v", n, nSwap)
+
        // If we've approved the contract to transfer, we can get a live
        // estimate to double check.

[INF] CORE[eth][DEXTT.ETH]: swapgas: 402 / 46289000

Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only some minor comments. All working well and code looks good.

The approval might eventually warrant a button in the wallets page because (a) it's not clear why live gas estimates don't work before approval until you look at the log, and (b) it's a bit dicey submitting a taker order before the approval tx is mined.

client/asset/driver.go Show resolved Hide resolved
client/webserver/http.go Outdated Show resolved Hide resolved
@buck54321
Copy link
Member Author

But I must be confused because the following gasses indicate we'd use and entire mainnet block with about a swap tx with ~265 inits in the batch. Even 10 lots, which is reasonable, would mean ~%4 of an entire block's gas limit? Can that be right?

I does seem limited, but considering that a regular tx is 21,000 gas, I think it's correct.

Just a reminder about #1426 too

var v0Gases = &dex.Gases{
	Swap:      135_000,
	SwapAdd:   113_000,
	Redeem:    63_000,
	RedeemAdd: 32_000,
	Refund:    43_000,
}

var v1Gases = &dex.Gases{
	Swap:      50_000,
	SwapAdd:   27_000,
	Redeem:    42_000,
	RedeemAdd: 14_000,
	// Refund: TODO,
}

return nil, fmt.Errorf("tx %s value < sum of inits. %d < %d", bc.txHash, bc.value, sum)
}
}

return &swapCoin{
baseCoin: bc,
init: init,
evmVal: evmVal,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this meant to be used and/or set to something other than 0?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops. Leftovers from intermediate iteration.

@chappjc
Copy link
Member

chappjc commented Aug 9, 2022

I does seem limited, but considering that a regular tx is 21,000 gas, I think it's correct.

Back to the gas estimate issue, I think this means we should place a cap on the number of inits.

More broadly, it sounds like we need to place a cap on the number of lots otherwise there will be potentially unswappable orders, however huge they'd be.

@chappjc
Copy link
Member

chappjc commented Aug 9, 2022

@martonp Pls circle back and we can merge.

@chappjc
Copy link
Member

chappjc commented Aug 11, 2022

Oh, was ready to merge, but there are stupid cache buster conflicts. If you can't get to that tonight I'll take care of it.

client/asset:
Add allowable networks to RegisterToken. The client can then use
the new package function SetNetwork during initialization to filter out
incompatible tokens.

client/asset/eth:
Change OpenTokenWallet to use a single struct argument. Add PeerChange
function and notify token wallets of peer changes.
Fix bugs in swap fee estimation to deal with transfer approval.

client/core:
Plumb in token wallet creation and opening.
Add TokenInfo field to SupportedAsset, populated instead of the
Info field for token wallets. Should consider changing the name
of the Info field to better illustrate the purpose and to break
Go consumer's API to force adaptation to the new semantics.

client/webserver:
Add token to live tests.

ui:
Implement new wallet creation flow for token wallets that includes
a step to sync the parent. Adapt to new SupportedAsset structure.
Add "token-aware symbols", which are symbols that, for tokens, have
an additional <sup> element denoting the parent asset.

testing:
Loadbot and core simnet tests are updated for dextt.eth.
Fix remaining gwei unit in shared dexeth.Initiation struct.
Fix busted nodeclient_harness_test.go tests.
Prevent double parent wallet creation.
@chappjc chappjc merged commit 74907fb into decred:master Aug 11, 2022
@chappjc chappjc added the ETH label Mar 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants