server/eth: Implement ValidateContract. #1273

JoeGruffins · 2021-11-05T06:57:45Z

server/eth: Implement ValidateContract.

Require the the Contract field of msgjson.Init be filled with the txdata
used to create the swap. Validate that txdata to be of the correct
format sending the expected types of arguments.

work towards #1154 depends on #1235

buck54321 · 2021-11-12T03:00:22Z

server/asset/eth/coiner.go

+
+// Auth ensures that a user really has the keys to spend a funding coin by
+// verifying a message signed by the funding coin address.
+func (c *amountCoin) Auth(pubkeys, sigs [][]byte, msg []byte) error {


I guess we'll still have to figure out when and how we confirm ownership of the account, but it won't be via coin like this anymore.

Yeah, this pr is only the last commit, so I'll rebase and remove that.

buck54321 · 2021-11-12T03:00:46Z

server/asset/eth/eth.go

+// contains valid counterparty, secret hash, and locktime.
+func (eth *Backend) ValidateContract(txdata []byte) error {
+	_, _, _, err := dexeth.ParseInitiateData(txdata)
+	if err != nil {
+		return fmt.Errorf("unable to parse contract txdata: %w", err)
+	}
+	return nil
 }


I think this is the essential part of this PR after the changes re #1154 / #1243.

Require the the Contract field of msgjson.Init be filled with the txdata used to create the swap. Validate that txdata to be of the correct format sending the expected types of arguments.

martonp · 2021-11-20T08:38:56Z

Why did you choose the contract to be the txdata? Now that we are doing batch initiations, the contract will represent multiple swaps.

chappjc · 2021-11-20T16:08:23Z

Why did you choose the contract to be the txdata? Now that we are doing batch initiations, the contract will represent multiple swaps.

I'd like to get on the same page here, so to provide context for discussion:

server gets init message with a coinID []byte and a contract []byte
performs basic validation of the coinID and contract. For the contract, using DCR as an example, this means decoding the script to ensure it's the right form of contract to perform a swap, not even checking the amounts, address, locktime, etc. For ETH, I believe we would just need to ensure that a known deployed swap contract is being used (one of the pre-validated contracts). There's only one in v0.

func (dcr *Backend) ValidateContract(contract []byte) error {
	_, _, _, _, err := dexdcr.ExtractSwapDetails(contract, chainParams)
	return err
}

after basic validation, server starts searching for the transaction on the network to actually check the swap details because ValidateContract does not do much here, using the Contract(coinID, contract) method
the result of finding it is the counterparty's address, lockTime, and amount of the particular swap (which keyed by secret hash, wherever we are encoding that with ETH now). For ETH, I'm not yet certain if this data should come out of the transaction call data, or wait until the contract account's state reflects the new init. In any case, if txData does have a role server-side (besides relaying that to the counter-party), it would come after Contract actually retrieves it. To me it looks like the ValidateContract with ETH should just check that the intended contract address is one of the ones known to the server i.e. a pre-validated contract.

chappjc · 2021-11-20T16:17:47Z

So, please bring me up to speed: what is the current coinID encoding and contract bytes in the init request? All that needs to be conveyed I think is: 1) the swap's unique secret hash, 2) the intended swap contract address, and 3) presumably the transaction and batch swap index to retrieve txData. Only (2) is used by ValidateContract, while all are used for Contract.
What is the current thinking about where all this data will be encoded?
The txData comes from the returned contract object from chain.Contract(params.CoinID, params.Contract), so it's definitely not going to be around for the preliminary ValidateContract call.

buck54321 · 2021-11-20T16:25:52Z

@chappjc see also #1154 (comment)

I recommend that for Init we send only secret hash = contract and txid = coin ID. Don't need an index, since we have the secret hash, I think. I made a comment here too, but I don't think we need any special encoding for "coin ID" = txid.

With that scheme, the only thing ValidateContract needs to do for ETH is to check the length of the secret hash.

chappjc · 2021-11-20T16:31:42Z

In that comment, @buck54321:

2 I'm going to propose something different for contract address validation, but if we do want to validate the contract address, it'll need to be done earlier than this anyway, so let's not plan to send it here.

What do you mean by "earlier than this"? Init is the start of the swap, so the contract address that's targeted for the swap would need to come here. Do you intend to pick it out of the transaction data after retrieving it via the txid? If so, this would come in processInit from the Contract method. Which is fine.

Will have a look at #1307

With that scheme, the only thing ValidateContract needs to do for ETH is to check the length of the secret hash.

I agree that ValidateContract can and should do very little at this stage (before finding the tx on-chain).

buck54321 · 2021-11-20T16:40:14Z

What do you mean by "earlier than this"? Init is the start of the swap, so the contract address that's targeted for the swap would need to come here.

We shouldn't even place an order on a server using an unsupported contract address. #1301 links the contract address to the dex.Asset.Version version.

chappjc · 2021-11-20T16:42:03Z

Yes, of course. But this is server-side validation. In some server checks (not ValidateContract), the contract address has to be checked. I think the right place to get that is from the tx data retrieved by the txid, but I want to understand your thinking on this.

Presumably the Contract method that tries to get all the swap data like counterparty address, swap amount, and locktime, will have to get that data from the transaction it gets via txid, but also validate the effect on the contract state at some point.

buck54321 · 2021-11-20T16:54:42Z

Presumably the Contract method that tries to get all the swap data like counterparty address, swap amount, and locktime, will have to get that data from the transaction it gets via txid, but also validate the effect on the contract state at some point.

Right. And the contract address doesn't need be decoded from the txdata. It should be the to address of the transaction.

chappjc · 2021-11-20T17:04:14Z

Great, we are 100% on the same page now. The contract address is validated when Contract verifies the txn in fact interacts with the right contract (the to address as you point out).
I also agree that ValidateAddress can do no more than check the length of the secret hash. Checking a provided swap contract address is pointless because it can be a lie. Anything else has to come out of the located transaction and its call data on-network, or the effects of the transaction on the contract state.

JoeGruffins · 2021-11-22T02:16:54Z

server/asset/eth/common.go

@@ -59,6 +59,9 @@ const (
 	// which holds the ETH, an amount of ETH in gwei, and a random nonce to
 	// avoid duplicate coin IDs.
 	CIDAmount
+	// SecretHashSize is the byte-length of the hash of the secret key used
+	// in swaps.
+	SecretHashSize = 32


Will be moved to dex/networks

JoeGruffins · 2021-11-22T02:20:00Z

The txData comes from the returned contract object from chain.Contract(params.CoinID, params.Contract), so it's definitely not going to be around for the preliminary ValidateContract call.

Whoops.

Now that we are doing batch initiations, the contract will represent multiple swaps.

Also whoops, but I guess we don't have that data yet anyway.

martonp

Looks good.

JoeGruffins changed the title ~~Servervalidatecontract~~ server/eth: Implement ValidateContract. Nov 5, 2021

buck54321 reviewed Nov 12, 2021

View reviewed changes

chappjc added the ETH label Nov 12, 2021

server/eth: Implement ValidateContract.

fb7bf35

Require the the Contract field of msgjson.Init be filled with the txdata used to create the swap. Validate that txdata to be of the correct format sending the expected types of arguments.

JoeGruffins force-pushed the servervalidatecontract branch from ce5fdf7 to fb7bf35 Compare November 19, 2021 07:01

JoeGruffins marked this pull request as ready for review November 19, 2021 07:02

Only check secret hash.

fb0edd7

JoeGruffins commented Nov 22, 2021

View reviewed changes

martonp approved these changes Nov 22, 2021

View reviewed changes

chappjc approved these changes Nov 22, 2021

View reviewed changes

chappjc merged commit 05367f1 into decred:master Nov 22, 2021

chappjc added this to the 0.5 milestone Nov 28, 2021

chappjc removed this from the 0.5 milestone Aug 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server/eth: Implement ValidateContract. #1273

server/eth: Implement ValidateContract. #1273

JoeGruffins commented Nov 5, 2021

buck54321 Nov 12, 2021

JoeGruffins Nov 12, 2021

buck54321 Nov 12, 2021

martonp commented Nov 20, 2021

chappjc commented Nov 20, 2021 •

edited

chappjc commented Nov 20, 2021 •

edited

buck54321 commented Nov 20, 2021

chappjc commented Nov 20, 2021 •

edited

buck54321 commented Nov 20, 2021 •

edited

chappjc commented Nov 20, 2021 •

edited

buck54321 commented Nov 20, 2021

chappjc commented Nov 20, 2021 •

edited

JoeGruffins Nov 22, 2021

JoeGruffins commented Nov 22, 2021

martonp left a comment

server/eth: Implement ValidateContract. #1273

server/eth: Implement ValidateContract. #1273

Conversation

JoeGruffins commented Nov 5, 2021

buck54321 Nov 12, 2021

Choose a reason for hiding this comment

JoeGruffins Nov 12, 2021

Choose a reason for hiding this comment

buck54321 Nov 12, 2021

Choose a reason for hiding this comment

martonp commented Nov 20, 2021

chappjc commented Nov 20, 2021 • edited

chappjc commented Nov 20, 2021 • edited

buck54321 commented Nov 20, 2021

chappjc commented Nov 20, 2021 • edited

buck54321 commented Nov 20, 2021 • edited

chappjc commented Nov 20, 2021 • edited

buck54321 commented Nov 20, 2021

chappjc commented Nov 20, 2021 • edited

JoeGruffins Nov 22, 2021

Choose a reason for hiding this comment

JoeGruffins commented Nov 22, 2021

martonp left a comment

Choose a reason for hiding this comment

chappjc commented Nov 20, 2021 •

edited

chappjc commented Nov 20, 2021 •

edited

chappjc commented Nov 20, 2021 •

edited

buck54321 commented Nov 20, 2021 •

edited

chappjc commented Nov 20, 2021 •

edited

chappjc commented Nov 20, 2021 •

edited