eth swap contract v1 #1426

buck54321 · 2022-01-19T00:53:08Z

I was just playing with this trying to minimize fees. I think it's sound, but there are some tradeoffs and some new edges that need exploring. Leaving in draft for the foreseeable future and focusing on token stuff.

Don't store contract data on-chain, just commit to it in the key of
the swaps map, which maps to a "swap record", 32 bytes in length,
with a special interpretation scheme that allows us to track the
swap status. This reduces fees by a factor of ~2 for a single-match
round-trip trade, which would be similar to e.g. ETH-USDC or other
token trade. For multi-match trades, the savings are even better,
asymptoting at ~28% of the v0 fees. A single-match round trip trade
uses ~ 93k gas, which is notably less than a trade on UniSwap and
other Ethereum trading platforms which are typically > 135k, I think.

JoeGruffins

Some comments on the contract.

JoeGruffins · 2022-01-19T02:30:09Z

dex/networks/eth/contracts/ETHSwapV1.sol

+            // Is this needed?
+            // require(msg.sender == c.initiator)


It would be required for refund, but as far as I know this would just lead to the initiator possibly shooting themselves in the foot.

This got me thinking so I made #1427

Yeah, it doesn't seem strictly necessary to me. I see this as limiting to 1) third-party services, and 2) privacy-focused clients.

JoeGruffins · 2022-01-19T03:43:58Z

dex/networks/eth/contracts/ETHSwapV1.sol

+    // contractKey generates a key hash which commits to the contract data. The
+    // generated hash is used as a key in the swaps map.
+    function contractKey(Contract calldata c) public pure returns (bytes32) {
+        return sha256(abi.encodePacked(c.secretHash, c.initiator, c.participant, c.value, c.refundTimestamp));
+    }


This obfuscates things a bit. All of this information is needed to reconstruct the key where it was just the secret hash before. If any is lost the swap wouldn't even be refundable. Say you lost all data and had to reseed. With version zero you could look at all the swaps and probably find your address as initiator with status refundable and wouldn't need the secret anymore. With this version, I guess the server would need to help the user out if they needed to refund in that scenario. Just playing devil's advocate here, it is a good idea to save space.

With version zero you could look at all the swaps and probably find your address as initiator with status refundable and wouldn't need the secret anymore.

Can you though? Swaps are keyed by secret hash, and there is no getter for all the swaps. If you nuke your db during settlement, aren't all bets are off? I feel like this is a just a general limitation of p2sh contracts that involve two parties.

there is no getter for all the swaps

I think you can get all the swaps, it's public. I know I did in the begging while testing.

I think this used to return all the swaps

dcrdex/dex/networks/eth/contracts/v0/contract.go

Lines 335 to 338 in 4548c2b

// Swaps is a free data retrieval call binding the contract method 0xeb84e7f2.

//

// Solidity: function swaps(bytes32 ) view returns(bytes32 secret, uint256 value, uint256 initBlockNumber, uint256 refundBlockTimestamp, address initiator, address participant, uint8 state)

func (_ETHSwap *ETHSwapCaller) Swaps(opts *bind.CallOpts, arg0 [32]byte) (struct {

But looks like it doesn't now?

Maybe we don't even need the Swap getter now?

Oh, I think you're right and I'm miss-remembering: https://ethereum.stackexchange.com/questions/15337/can-we-get-all-elements-stored-in-a-mapping-in-the-contract

So, one wouldn't be able to find their swap without the key/secret hash

JoeGruffins · 2022-01-19T03:52:06Z

dex/networks/eth/contracts/ETHSwapV1.sol

+    //   else if (uint256(record) < block.number && sha256(record) != contract.secret):
+    //      contract is initiated and redeemable by the participant with the secret.
+    //   else if (sha256(record) == contract.secret): contract has been redeemed


contract.secret is contract.secretHash?

If secret was less than the block number and hashed to the secret hash, it would never be redeemable?

JoeGruffins · 2022-01-19T04:51:07Z

dex/networks/eth/contracts/ETHSwapV1.sol

+            bytes32 record = swaps[k];
+            require(record == bytes32(0), "swap not empty");
+
+            swaps[k] = bytes32(block.number);


I think you need to make sure this does not hash to secret hash. One could predict the block this gets mined (or is this the block before? I've forgotten).

JoeGruffins · 2022-01-19T04:56:05Z

dex/networks/eth/contracts/ETHSwapV1.sol

+        if (secretValidates(record, c.secretHash)) {
+            return State.Redeemed;
+        }
+        // Is it worth checking whether blockNum < block.number?


I would say no.

JoeGruffins · 2022-01-19T05:00:09Z

dex/networks/eth/contracts/ETHSwapV1.sol

+            // To be redeemable, the record needs to represent a valid block
+            // number.
+            require(blockNum > 0 && blockNum < block.number, "unfilled swap");
+            // require(blockNum >> 32 == 0, "invalid swap format");


Commented out, but does ethereum have this limitation of a uint32 block number?

No, but it'll take a couple hundred years to get to that block height. This comment is left over from an earlier iteration on the record scheme, but blockNum < block.number is better.

chappjc · 2022-09-05T21:15:36Z

client/asset/eth/contractor.go

+// This one breaks contractor, since we need the entire *asset.Contract, not just
+// the secret hash.


There are quite a few broken like this, including swap, which is used all over. e.g. https://github.com/decred/dcrdex/pull/1638/files#diff-a45b0f60d5cf2f6825eb9e9ed1f173fbf1df9eb53b09914ce6b17e42074653e5R101

I'm confused about how we're going to adapt if we can't look up the Swap struct by secret hash any more.

It also becomes super hard to use the refund-by-anyone path we are relying on as a backup. When we create the swap we can't a pre-signed refund, so we have been logging the secret hash so the user can just (e.g.) take that to etherscan and refund with metamask.

I'm confused about how we're going to adapt if we can't look up the Swap struct by secret hash any more.

We could do it with the Swaps and the ContractKey methods. We would just need to interpret ourselves. But I've added a little more detail to the return from state (renamed to status) to make things easier. Happy to go back if you're concerned with the contract change.

It also becomes super hard to use the refund-by-anyone path we are relying on as a backup. When we create the swap we can't a pre-signed refund, so we have been logging the secret hash so the user can just (e.g.) take that to etherscan and refund with metamask.

I'll look into a solution, but I don't see any reason that something like this should stop us.

buck54321 · 2022-09-05T22:30:02Z

client/asset/interface.go

@@ -326,7 +330,7 @@ type Wallet interface {
 	// NOTE: This could potentially be a long and expensive operation if
 	// performed long after the swap is broadcast; might be better executed from
 	// a goroutine.
-	FindRedemption(ctx context.Context, coinID, contract dex.Bytes) (redemptionCoin, secret dex.Bytes, err error)
+	FindRedemption(ctx context.Context, coinID, contractData dex.Bytes, deets *dex.SwapContractDetails) (redemptionCoin, secret dex.Bytes, err error)


I was on the fence about adding these arguments, because I think we could probably work it out without them, through the tx cache or something. But all of the info in the SwapContractDetails is required to be saved by the client already, so it actually seems more on-model to have Core supply it. Open to suggestions though.

I agree. We should not rely on the backend somehow having recorded it previously. I think all the method should be able to do their job with the inputs plus what they find on-chain.

buck54321 · 2022-09-05T22:31:41Z

server/asset/eth/tokener.go

-	es, err := erc20v0.NewERC20Swap(swapContract.Address, be)
+	es, err := erc20v1.NewERC20Swap(swapContract.Address, be)


I had always intended for the server to only support a single version at a time, and I still do. But there's probably an argument for making it switchable via configuration. I don't know.

Although it would seem crazy to us, a user will update in the middle of a swap. Or perhaps a redeem or refund they forgot about. It would be responsible to continue to support old versions and deprecate slowly.

I would expect the client would be able to refund or auto-redeem those on their own.

Slightly related, which I am still intending to address myself: #1583

Oh, this is the server, I wanted to make this comment on the client.

Oh yeah, we are handling both versions for the client. Please ignore my comment.

I guess markets will just need to be suspended for makerlocktime to ensure no new initiations? Or are clients handling this fine already? And hopefully refunds will still be ok, I think ok. Should be ok.

buck54321 · 2022-09-05T22:34:19Z

client/asset/interface.go

 	// Spends is the AuditInfo for the swap output being spent.
 	Spends *AuditInfo
+	// SwapDetails is the swap details.
+	SwapDetails *dex.SwapContractDetails


I'd like to refactor this later. SwapContractDetails encodes all of AuditInfo except for the Contract field. But I didn't want to mess with the btc/dcr interfaces too much in the PR.

buck54321 · 2022-09-05T23:33:37Z

server/asset/eth/coiner.go

-	// Uninitiated state is zero confs. It could still be in mempool.
-	// It is important to only trust confirmations according to the
-	// swap contract. Until there are confirmations we cannot be sure
-	// that initiation happened successfully.
-	if swap.State == dexeth.SSNone {
-		// Assume the tx still has a chance of being mined.
-		return 0, nil
-	}


It wasn't quite clear to me why we wouldn't return CoinNotFoundError here. We have the latency queue running for (Backend).Confirmations in (Swapper).processInit.

I don't remember. CoinNotFoundError is probably correct.

I believe it's because the contract state will not be reflecting txns in txpool. As such, there is ambiguity here about whether it's really not found or if it is in fact in txpool. What is the problem with returning 0, nil? That would be correct for a transaction not yet mined.

buck54321 · 2022-09-05T23:35:29Z

server/asset/eth/coiner.go

 func (c *swapCoin) Confirmations(ctx context.Context) (int64, error) {
-	swap, err := c.backend.node.swap(ctx, c.backend.assetID, c.secretHash)
-	if err != nil {
-		return -1, err
-	}
-
-	// Uninitiated state is zero confs. It could still be in mempool.


I've moved these functions away from contract inspection and towards more transaction inspection. I think it simplifies things. Any reason I can't do that?

Yeah, I don't recall why the Confirmations methods were so contract state oriented. Neither do I remember why dexeth.SSNone should not be a CoinNotFoundError. Perhaps @JoeGruffins recalls.

I wanted to ensure that this info comes from the contract and not our assumptions about the tx. Especially with the recent redemptions pr we may be looking at a bogus transaction. (I realize this is the init one but still)

If you are sure we are not then fine, but I'm not sure we couldn't have a tx that passed our verification but does not do what we expect. Our txdata parsing is basically a hack, we had to copy paste from geth internals. I do not think it is intended to be done this way.

At least there needs to be another check besides confirmations that checks status is ok, as in it did what we expected onchain. But, I hope we can not trust the transaction except for the basic sanity check we were doing and start looking at the contract here.

It's certainly not the eth way. Things are meant to replaceable, and the focus is supposed to be on state change, and contract interactions.

Is there an reason for just checking the txid and not getting confs from the contract?

On the topic Joe brings up, I'd feel better about doing less tx inspection with the hacked call data parsing if possible.

I worry we're already dug in on something we shouldn't be doing as well.

chappjc · 2022-09-06T12:56:23Z

client/asset/ltc/ltc.go

@@ -119,6 +119,7 @@ var (
 			rpcWalletDefinition,
 			electrumWalletDefinition,
 		},
+		ProtocolVersions: []uint32{0},


line 17 has version = 1. Is this yet another version or the same?

// ProtocolVersions is the Wallet's accepted server version numbers.

And what was version? That's exactly what I thought it was.

You're right. Sorry. Core was using it like you say. Should I get rid of Version?

Whatever you prefer. I was just confused that there was something else being tracked here.

chappjc · 2022-09-06T13:04:54Z

client/asset/btc/btc.go

-		return false, time.Time{}, fmt.Errorf("error extracting contract locktime: %w", err)
-	}
-	contractExpiry := time.Unix(int64(locktime), 0).UTC()
+func (btc *baseWallet) LocktimeExpired(_ context.Context, deets *dex.SwapContractDetails) (bool, time.Time, error) {


One thing I really hate is when we have function arguments with way more info than is needed for a sensible implementation.

For BTC, say you're trying to use LocktimeExpored or Refund or whatever, so you're prepping to call the baseWallet method... you think, what fields of SwapContractDetails does it really need?
I get why. We're gravitating toward an interface that fits all assets and ETH is getting very different now.
But what can we do to avoid this? Obviously docs can be very clear for each asset to call out the used data, at a minimum.

what fields of SwapContractDetails does it really need?

I'm with you on the API, but I'm not adding any more than is needed here. We could cache the data internally, but trying to avoid that.

I mean it's way more than needed for BTC.

Why not stick all the data from SwapContractDetails into the contract dex.Bytes?

That sounds appealing to me too.

client/asset/eth/contractor.go

Don't store contract data on-chain, just commit to it in the key of the swaps map, which maps to a "swap record", 32 bytes in length, with a special interpretation scheme that allows us to track the swap status. This reduces fees by a factor of ~2 for a single lot round-trip trade, which would be similar to e.g. ETH-USDC or other token trade. For many-lot trades, the savings are even better, asymptoting at ~28% of the v0 fees. A single-lot round trip trade uses ~ 93k gas, which is notably less than a trade on UniSwap and other Ethereum trading platforms.

Add support for new contracts through server/asset/eth and client/asset/eth. Requires some modifications to client/core and client/asset to add more swap info to some methods. Differentiate wallet version from supported server versions.

client/core/trade.go

JoeGruffins · 2022-09-08T05:10:56Z

client/asset/eth/contractor.go

+	var secretHash [32]byte
+	copy(secretHash[:], contract.SecretHash)


I see this copy a lot. Maybe contract.SecretHash could just be a [32]byte? Underneath it would still copy but be a few lines less code.

JoeGruffins · 2022-09-08T05:35:48Z

client/asset/eth/contractor.go

+
+func newV1Contractor(net dex.Network, acctAddr common.Address, cb bind.ContractBackend) (contractor, error) {
+	contractAddr, exists := dexeth.ContractAddresses[1][net]
+	if !exists || contractAddr == (common.Address{}) {


Do the () do something here?

JoeGruffins · 2022-09-08T05:46:41Z

client/asset/eth/contractor.go

+		value = dexeth.GweiToWei(uint64(n))
+	}
+
+	return c.estimateGas(ctx, value, "initiate", initiations)


nit: The strings "initiate", "redeem", "refund" are used quite a bit, could be constants to avoid spelling errors.

JoeGruffins · 2022-09-08T05:53:23Z

client/asset/eth/contractor.go

+	}
+	var secretHash [32]byte
+	copy(secretHash[:], deets.SecretHash)
+	return sha256.Sum256(secret[:]) == secretHash, nil


I would think it's a little late to do this check. We should be certain before the swap gets this far.

JoeGruffins · 2022-09-08T06:11:37Z

client/asset/eth/contractor.go

+// readOnlyCallOpts is the CallOpts used for read-only contract method calls.
+func readOnlyCallOpts(ctx context.Context) *bind.CallOpts {
+	return &bind.CallOpts{
+		Pending: true,


I think all or none of the call opts should have pending set true. Others in this file are not set.

JoeGruffins · 2022-09-08T08:55:13Z

dex/networks/eth/contracts/ETHSwapV1.sol

+
+            // To be redeemable, the record needs to represent a valid block
+            // number.
+            require(blockNum > 0 && blockNum < block.number, "unfilled swap");


uninitiated or refunded swap. It is also most redeemed swaps because secret would be a number > block.number

I forget now but probably fine to use the isRedeemable in place of this and the check below.

JoeGruffins · 2022-09-08T09:02:16Z

dex/networks/eth/contracts/ETHSwapV1.sol

+            record = bytes32(block.number);
+            require(!secretValidates(record, c.secretHash), "hash collision");


Although just as impossible as this collision, the secret can also not be RefundRecord. You could find out what that hashes to and do a straight != with the constant and record if you wanted to.

JoeGruffins · 2022-09-08T09:17:13Z

dex/networks/eth/contracts/ETHSwapV1.sol

+        // Is this swap initialized?
+        require(blockNum > 0 && blockNum <= block.number, "swap not active");


I think this makes the refunded check not reachable if refunded. It also stops most checks for is redeemed. Both of which are fine, but can remove the explicit refund check I guess, or move it and the redeem check up if the error is important.

JoeGruffins · 2022-09-08T09:26:19Z

dex/networks/eth/txdata.go

+const (
+	initiateFuncName = "initiate"
+	redeemFuncName   = "redeem"
+	refundFuncName   = "refund"
+)


Maybe export these? they are used a bunch in the clients.

JoeGruffins · 2022-09-08T09:28:21Z

dex/networks/eth/txdata.go

+	}
+	args := decoded.inputs
+	// Any difference in number of args and types than what we expect
+	// should be caught by parseCallData, but checking again anyway.


parseCallData -> ParseCallData

JoeGruffins · 2022-09-09T04:02:22Z

dex/networks/eth/txdata.go

+// specific version of the swap contract. It returns the the list of initiations
+// done in the call and errors if the call data does not call initiate initiate


the twice in It returns the the and initiate twice in does not call initiate initiate

JoeGruffins · 2022-09-09T06:33:55Z

dex/networks/eth/txdata.go

+	// This is done for the compiler to ensure that the type defined above and
+	// swapv0.ETHSwapRedemption are the same, other than the tags.
+	if len(redemptions) > 0 {
+		// Why can't I do ETHSwapRedemption directly?
+		_ = swapv1.ETHSwapContract(redemptions[0].C)
+	}


I guess the nested types cannot be casted https://go.dev/play/p/8PBsInPWbCz

It looks like this works:

if len(redemptions) > 0 { _ = swapv1.ETHSwapRedemption(struct { C swapv1.ETHSwapContract Secret [32]uint8 }{ C: swapv1.ETHSwapContract(redemptions[0].C), Secret: redemptions[0].Secret, }) }

Could you make tests like there are for v1?

JoeGruffins · 2022-09-09T06:45:53Z

dex/networks/eth/txdata.go

+	contract, ok := args[0].value.(struct {
+		SecretHash      [32]byte       `json:"secretHash"`
+		Initiator       common.Address `json:"initiator"`
+		RefundTimestamp uint64         `json:"refundTimestamp"`
+		Participant     common.Address `json:"participant"`
+		Value           uint64         `json:"value"`
+	})


Could add the check like the others that these are the same fields as swapv1.ETHSwapContract

JoeGruffins · 2022-09-09T06:46:13Z

dex/networks/eth/txdata.go

+		Value           uint64         `json:"value"`
+	})
+	if !ok {
+		return nil, fmt.Errorf("expected first arg of type [32]byte but got %T", args[0].value)


not a [32]byte but swapv1.ETHSwapContract I guess

JoeGruffins · 2022-09-09T06:58:46Z

server/asset/eth/coiner.go

-	if err == asset.CoinNotFoundError {
-		// If the coin is not found, check to see if the swap has been
-		// redeemed by another transaction.
-		contractVer, secretHash, err := dexeth.DecodeContractData(contractData)
-		if err != nil {
-			return nil, err
-		}
-		be.log.Warnf("redeem coin with ID %x for secret hash %x was not found", coinID, secretHash)
-		swapState, err := be.node.swap(be.ctx, be.assetID, secretHash)
-		if err != nil {
-			return nil, err
-		}
-		if swapState.State != dexeth.SSRedeemed {
-			return nil, asset.CoinNotFoundError
-		}
-		bc = &baseCoin{
-			backend:     be,
-			secretHash:  secretHash,
-			contractVer: contractVer,
-		}
-		return &redeemCoin{
-			baseCoin: bc,
-			secret:   swapState.Secret,
-		}, nil
-	} else if err != nil {
-		return nil, err


Maybe this was a rebase mixup? I think we need this because of the redemption pr recently. The client may have redeemed with a different tx.

JoeGruffins · 2022-09-09T07:03:46Z

server/asset/eth/coiner.go

 func (c *swapCoin) Confirmations(ctx context.Context) (int64, error) {
-	swap, err := c.backend.node.swap(ctx, c.backend.assetID, c.secretHash)
-	if err != nil {
-		return -1, err
-	}
-
-	// Uninitiated state is zero confs. It could still be in mempool.


I wanted to ensure that this info comes from the contract and not our assumptions about the tx. Especially with the recent redemptions pr we may be looking at a bogus transaction. (I realize this is the init one but still)

If you are sure we are not then fine, but I'm not sure we couldn't have a tx that passed our verification but does not do what we expect. Our txdata parsing is basically a hack, we had to copy paste from geth internals. I do not think it is intended to be done this way.

At least there needs to be another check besides confirmations that checks status is ok, as in it did what we expected onchain. But, I hope we can not trust the transaction except for the basic sanity check we were doing and start looking at the contract here.

JoeGruffins · 2022-09-09T07:11:26Z

server/asset/eth/coiner.go

-	// Uninitiated state is zero confs. It could still be in mempool.
-	// It is important to only trust confirmations according to the
-	// swap contract. Until there are confirmations we cannot be sure
-	// that initiation happened successfully.
-	if swap.State == dexeth.SSNone {
-		// Assume the tx still has a chance of being mined.
-		return 0, nil
-	}


I don't remember. CoinNotFoundError is probably correct.

JoeGruffins · 2022-09-09T07:23:36Z

server/asset/eth/tokener.go

-	es, err := erc20v0.NewERC20Swap(swapContract.Address, be)
+	es, err := erc20v1.NewERC20Swap(swapContract.Address, be)


Although it would seem crazy to us, a user will update in the middle of a swap. Or perhaps a redeem or refund they forgot about. It would be responsible to continue to support old versions and deprecate slowly.

martonp

Looks good overall. I think the API would be more simple if all the dex.SwapContractDetails stuff was just in the contract bytes. All of the information in there is available when the contract bytes are created in Swap.

martonp · 2022-09-06T03:46:51Z

dex/networks/erc20/contracts/ERC20SwapV1.sol

+// pragma should be as specific as possible to allow easier validation.
+pragma solidity = 0.8.15;
+
+// ETHSwap creates a contract to be deployed on an ethereum network. In


ERC20Swap creates a contract. It's also wrong in v0.

martonp · 2022-09-06T04:47:28Z

dex/networks/eth/contracts/ETHSwapV1.sol

+    }
+
+    // redeem redeems a Contract. It checks that the sender is not a contract,
+    // and that the secret hash hashes to secretHash. msg.value is tranfered


It's not msg.value that is transferred, but the sum of the redemptions' values.

martonp · 2022-09-06T05:01:39Z

dex/networks/eth/contracts/ETHSwapV1.sol

+            require(secretValidates(r.secret, r.c.secretHash), "invalid secret");
+
+            swaps[k] = r.secret;
+            amountToRedeem += r.c.value * 1 gwei;


Why not pass the value in wei instead of the value in gwei? Isn't this extra multiplication a waste of gas?

martonp · 2022-09-06T10:22:46Z

client/asset/eth/eth.go

+		return 0
+	case 1:
+		return 1
+	case contractVersionNewest:


The contractVersionNewest case should actually return the newest version. And shouldn't the default case just return an error? How can an unknown version be used?

martonp · 2022-09-14T06:15:31Z

client/asset/btc/btc.go

-		return false, time.Time{}, fmt.Errorf("error extracting contract locktime: %w", err)
-	}
-	contractExpiry := time.Unix(int64(locktime), 0).UTC()
+func (btc *baseWallet) LocktimeExpired(_ context.Context, deets *dex.SwapContractDetails) (bool, time.Time, error) {


Why not stick all the data from SwapContractDetails into the contract dex.Bytes?

Implements the version 1 contracts for ethereum and tokens. Based on feedback in decred#1426, everything is now encoded in the "contract data". This "contract data", is the msgjson.Init.Contract -> msgjson.Audit.Contract -> MatchMetaData.Proof.CounterContract, AuditInfo.Contract -> Redemption.Spends.Contract. A few new terms are introduced to differentiate various encodings and data sets. The aforementioned contract data did encode a version and a secret hash. It now encodes a version and a "locator", which is a []byte whose length and content depend on the version. For version 0, the locator is still just the secretHash[:]. For v1, the locator encodes all of the immutable data that defines the swap. This immutable data is now collected in something called a "vector" (dexeth.SwapVector). For version 0, some vector data is stored on-chain indexed by the secret hash. For version 1, all vector data is encoded in the locator. I've also made an effort to standardize the use of status/step, and eliminated the use of ambiguous "ver" variables throughout. A "status" is now the collection of mutable contract data: the step, the init block height, and the secret. The status and vector collectively fully characterize the swap. client/asset/eth: New contractV1 and tokenContractorV1 interfaces. To avoid duplication, the ERC20 parts of the tokenContractors are separated into a new type erc20Contractor that is embedded by both versions. Getters for status and vector are added in place of the old method "swap". assetWallet and embedding types are updated to work with the new version-dependent locators and the status and vector model. dex/networks/{eth,erc20}: New contracts added. New methods for dealing with locators. Simnet entries added for eth and dextt.eth in the ContractAddresses and Tokens maps. txDataHandler interace is replaced with versioned package-level functions. server/asset/eth: Server is fully switched to version 1. No option to use version 0. Translation to new version was straightforward, with one notable difference that we can no longer get a block height from the contract once the swap is redeemed.

buck54321 · 2023-01-10T18:58:57Z

Closing in favor of #2038

Implements the version 1 contracts for ethereum and tokens. Based on feedback in decred#1426, everything is now encoded in the "contract data". This "contract data", is the msgjson.Init.Contract -> msgjson.Audit.Contract -> MatchMetaData.Proof.CounterContract, AuditInfo.Contract -> Redemption.Spends.Contract. A few new terms are introduced to differentiate various encodings and data sets. The aforementioned contract data did encode a version and a secret hash. It now encodes a version and a "locator", which is a []byte whose length and content depend on the version. For version 0, the locator is still just the secretHash[:]. For v1, the locator encodes all of the immutable data that defines the swap. This immutable data is now collected in something called a "vector" (dexeth.SwapVector). For version 0, some vector data is stored on-chain indexed by the secret hash. For version 1, all vector data is encoded in the locator. I've also made an effort to standardize the use of status/step, and eliminated the use of ambiguous "ver" variables throughout. A "status" is now the collection of mutable contract data: the step, the init block height, and the secret. The status and vector collectively fully characterize the swap. client/asset/eth: New contractV1 and tokenContractorV1 interfaces. To avoid duplication, the ERC20 parts of the tokenContractors are separated into a new type erc20Contractor that is embedded by both versions. Getters for status and vector are added in place of the old method "swap". assetWallet and embedding types are updated to work with the new version-dependent locators and the status and vector model. dex/networks/{eth,erc20}: New contracts added. New methods for dealing with locators. Simnet entries added for eth and dextt.eth in the ContractAddresses and Tokens maps. txDataHandler interace is replaced with versioned package-level functions. server/asset/eth: Server is fully switched to version 1. No option to use version 0. Translation to new version was straightforward, with one notable difference that we can no longer get a block height from the contract once the swap is redeemed.

buck54321 force-pushed the contract-versions branch 2 times, most recently from d3fc284 to 1f0da0d Compare January 19, 2022 01:23

JoeGruffins reviewed Jan 19, 2022

View reviewed changes

buck54321 mentioned this pull request Aug 9, 2022

multi: enable token trading #1622

Merged

chappjc reviewed Sep 5, 2022

View reviewed changes

buck54321 force-pushed the contract-versions branch from 0458637 to 240ed63 Compare September 5, 2022 22:11

buck54321 commented Sep 5, 2022

View reviewed changes

buck54321 force-pushed the contract-versions branch from 240ed63 to b545910 Compare September 5, 2022 23:47

chappjc reviewed Sep 6, 2022

View reviewed changes

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

buck54321 added 5 commits September 6, 2022 15:31

touchups

d9a33ae

checkpoint

cb22720

finish implementation

53b08ac

Add support for new contracts through server/asset/eth and client/asset/eth. Requires some modifications to client/core and client/asset to add more swap info to some methods. Differentiate wallet version from supported server versions.

cleanup and early review followup

8624e81

buck54321 force-pushed the contract-versions branch from b545910 to 8624e81 Compare September 6, 2022 23:41

buck54321 marked this pull request as ready for review September 6, 2022 23:49

chappjc reviewed Sep 7, 2022

View reviewed changes

client/core/trade.go Show resolved Hide resolved

JoeGruffins reviewed Sep 8, 2022

View reviewed changes

JoeGruffins reviewed Sep 9, 2022

View reviewed changes

martonp reviewed Sep 14, 2022

View reviewed changes

chappjc added this to the TBD milestone Sep 14, 2022

chappjc added the ETH label Oct 12, 2022

buck54321 mentioned this pull request Jan 10, 2023

eth: v1 contract #2038

Open

buck54321 closed this Jan 10, 2023

chappjc removed this from the TBD milestone Jan 11, 2023

	// Swaps is a free data retrieval call binding the contract method 0xeb84e7f2.
	//
	// Solidity: function swaps(bytes32 ) view returns(bytes32 secret, uint256 value, uint256 initBlockNumber, uint256 refundBlockTimestamp, address initiator, address participant, uint8 state)
	func (_ETHSwap ETHSwapCaller) Swaps(opts bind.CallOpts, arg0 [32]byte) (struct {

		// This one breaks contractor, since we need the entire *asset.Contract, not just
		// the secret hash.

		es, err := erc20v0.NewERC20Swap(swapContract.Address, be)
		es, err := erc20v1.NewERC20Swap(swapContract.Address, be)

		var secretHash [32]byte
		copy(secretHash[:], contract.SecretHash)

		record = bytes32(block.number);
		require(!secretValidates(record, c.secretHash), "hash collision");

		// Is this swap initialized?
		require(blockNum > 0 && blockNum <= block.number, "swap not active");

		// specific version of the swap contract. It returns the the list of initiations
		// done in the call and errors if the call data does not call initiate initiate

eth swap contract v1 #1426

eth swap contract v1 #1426

Conversation

buck54321 commented Jan 19, 2022 • edited Loading

JoeGruffins left a comment

Choose a reason for hiding this comment

JoeGruffins Jan 19, 2022 • edited Loading

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

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 Sep 5, 2022 • edited Loading

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 Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeGruffins Sep 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeGruffins Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeGruffins Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

buck54321 Sep 5, 2022 • edited Loading

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

JoeGruffins Sep 9, 2022 • edited Loading

Choose a reason for hiding this comment

chappjc Sep 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Sep 6, 2022 • edited Loading

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

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 Sep 9, 2022 • edited Loading

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 Sep 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeGruffins Sep 9, 2022 • edited Loading

buck54321 commented Jan 19, 2022 •

edited

Loading

JoeGruffins Jan 19, 2022 •

edited

Loading

chappjc Sep 5, 2022 •

edited

Loading

chappjc Sep 6, 2022 •

edited

Loading

JoeGruffins Sep 9, 2022 •

edited

Loading

JoeGruffins Sep 12, 2022 •

edited

Loading

JoeGruffins Sep 12, 2022 •

edited

Loading

buck54321 Sep 5, 2022 •

edited

Loading

JoeGruffins Sep 9, 2022 •

edited

Loading

chappjc Sep 9, 2022 •

edited

Loading

chappjc Sep 6, 2022 •

edited

Loading

chappjc Sep 6, 2022 •

edited

Loading

JoeGruffins Sep 9, 2022 •

edited

Loading

JoeGruffins Sep 9, 2022 •

edited

Loading

JoeGruffins Sep 9, 2022 •

edited

Loading