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

eth: v1 contract #2038

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

eth: v1 contract #2038

wants to merge 1 commit into from

Conversation

buck54321
Copy link
Member

@buck54321 buck54321 commented Jan 10, 2023

Replaces #1426. Not tested with simnet-trade-tests (#2037) or loadbot, so leaving in draft for now. Need to get some reviews done.

Implements the version 1 contracts for ethereum and tokens. Based
on feedback in #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 state.

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 buck54321 changed the title eth contract v1 eth: v1 contract Jan 10, 2023
@buck54321 buck54321 mentioned this pull request Jan 10, 2023
Comment on lines 2806 to 3422
spent = status.Step >= dexeth.SSRedeemed
confs = uint32(hdr.Number.Uint64() - status.BlockHeight + 1)

// NOTE: confs will equal to the block number for the version 1 contract if spent == true,
// because BlockHeight will be zero. This is probably fine, since the caller
// will examine spent first? Otherwise, we could do this.
Copy link
Member

Choose a reason for hiding this comment

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

Interesting problem to have here, but I think it's right to be using what's recorded in the contract state like you're doing in this version of the PR, at least before it's spent. What you suggest about getting the tx confirmations after it is spent might be alright unless the tx was replaced.

Comment on lines 2824 to 3436
// Or maybe it'd be better to set confs to some constant, large, round
// number so it doesn't look dumb in logs.
Copy link
Member

Choose a reason for hiding this comment

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

Yah, maybe like 32 or something?

@chappjc chappjc added this to the 1.0 milestone Jan 11, 2023
@chappjc chappjc added the ETH label Jan 12, 2023
@martonp
Copy link
Contributor

martonp commented Apr 11, 2023

There is a new ETH functionality that would allow us to have ETH redemptions for accounts without any ETH:
https://eips.ethereum.org/EIPS/eip-4337

Would be cool to have it in the v1 contract.

@norwnd
Copy link
Contributor

norwnd commented Jul 21, 2023

There is a new ETH functionality that would allow us to have ETH redemptions for accounts without any ETH:
https://eips.ethereum.org/EIPS/eip-4337

It might also allow for removal of additional "approve" step since multiple user actions can be executed within single transaction (from https://youtu.be/iLf8qpOmxQc?t=2028).

But who knows how long it will take to get this Account Abstraction design working, I bet it's 1+ year down the road.

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.

Swaps seem to be working fine, but I'm getting some errors on the server:

Screenshot 2024-03-13 at 8 19 23 PM

dex/networks/eth/contracts/ETHSwapV1.sol Outdated Show resolved Hide resolved
require(!secretValidates(record, v.secretHash), "swap already redeemed");

// Is it already refunded?
require(record != RefundRecord, "swap already refunded");
Copy link
Contributor

Choose a reason for hiding this comment

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

        require(blockNum > 0 && blockNum <= block.number, "swap not active");

If this check passes then record != RefundRecord will definitely be true.

// 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(Vector calldata v) public pure returns (bytes32) {
return sha256(bytes.concat(v.secretHash, bytes20(v.initiator), bytes20(v.participant), bytes32(v.value), bytes8(v.refundTimestamp)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return sha256(bytes.concat(v.secretHash, bytes20(v.initiator), bytes20(v.participant), bytes32(v.value), bytes8(v.refundTimestamp)));
return keccak256(abi.encode(v));

I'm pretty sure keccak256 will be cheaper, and maybe abi.encode would also be cheaper than the bytes.concat.

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 was originally using abi.encodePacked, but we decided to use bytes.encode because of some ambiguity in the implementation of abi encoding, iirc. I can't find the convo. I think it was on Matrix.

Copy link
Member Author

@buck54321 buck54321 Mar 26, 2024

Choose a reason for hiding this comment

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

I still need to investigate keccak hash

client/asset/eth/eth.go Outdated Show resolved Hide resolved
dex/networks/eth/params.go Show resolved Hide resolved
server/asset/eth/coiner.go Show resolved Hide resolved
LockTime: uint64(init.LockTime.Unix()),
}

// if value < bc.vector.Value {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these checks be copied below to use with both versions?

Copy link
Member Author

Choose a reason for hiding this comment

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

The value check is moved and the others are completely pointless since we just pulled (v0) or validated (v1) the data on-chain. This is also not the purview of the backend, which reports chain data to the swapper for validation.

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.

Glad you rebased all of this. The gas savings on everything but Refund is substantial. I think I have a valid comment about the RefundRecord being used as the Secret and maybe some unintended things can happen with that.

client/asset/eth/contractor.go Outdated Show resolved Hide resolved
client/asset/eth/contractor.go Outdated Show resolved Hide resolved
Comment on lines +658 to +673
// func (c *contractorV1) record(v *dexeth.SwapVector) (r [32]byte, err error) {
// abiVec := dexeth.SwapVectorToAbigen(v)
// ck, err := c.ContractKey(&bind.CallOpts{From: c.acctAddr}, abiVec)
// if err != nil {
// return r, fmt.Errorf("ContractKey error: %v", err)
// }
// return c.Swaps(&bind.CallOpts{From: c.acctAddr}, ck)
// }
Copy link
Member

Choose a reason for hiding this comment

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

Ok to remove?

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 though this might come in handy in an emergency for troubleshooting. The underlying Swaps method is generated automatically by abigen anyway.

client/asset/eth/contractor.go Outdated Show resolved Hide resolved
client/asset/eth/eth.go Show resolved Hide resolved
server/asset/eth/coiner.go Show resolved Hide resolved
server/asset/eth/tokener.go Outdated Show resolved Hide resolved
server/asset/eth/tokener.go Outdated Show resolved Hide resolved
server/asset/eth/tokener.go Outdated Show resolved Hide resolved
Comment on lines +79 to +82
// From: ,
To: init.Participant,
Copy link
Member

Choose a reason for hiding this comment

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

Leaving from out on purpose? Also here is another example of To as Participant, which I think it correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

We simply don't have this information at this point, and luckily the server doesn't need it. I wasn't sure what else to do but leave it empty.

Comment on lines 2 to 9
"eth": 0,
"usdc.eth": 0,
"polygon": 0,
"usdc.polygon": 0,
"wbtc.polygon": 0,
"weth.polygon": 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.

This is annoying, but until we launch contracts, you'll have to delete these overrides to test v1 contracts.

@JoeGruffins
Copy link
Member

Is it possible to add tests for the contracts like @martonp has done in #2717 ?

@buck54321
Copy link
Member Author

All v1 contracts launched and gases validated on mainnet, and contracts launched for all available tokens on testnet.

Comment on lines +41 to +44
// else if (uint256(record) < block.number && sha256(record) != contract.secretHash):
// contract is initiated and redeemable by the participant with the secret.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use 20 bytes here for the the token address and the remaining 12 for the block number? Then we wouldn't need to create a new contract for each token.

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
Copy link
Member Author

Switched up to using a single contract for eth and tokens. Non-live unit tests are passing. Getgas utility is working. But not tested beyond that. This should be close to the final form though.

bytes32 secret;
}

function secretValidates(bytes32 secret, bytes32 secretHash) public pure returns (bool) {
Copy link
Contributor

@dev-warrior777 dev-warrior777 Nov 13, 2024

Choose a reason for hiding this comment

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

nit: I would prefer isSecretValidated for readabilty

Comment on lines +140 to +142
require(v.value > 0, "0 val");
require(v.refundTimestamp > 0, "0 refundTimestamp");
require(v.secretHash != RefundRecordHash, "illegal secret hash (refund record hash)");
Copy link
Contributor

Choose a reason for hiding this comment

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

0 val and refund hash can be checked by client. They can't result in the initiator cheating.

require(record == bytes32(0), "swap not empty");

record = bytes32(block.number);
require(!secretValidates(record, v.secretHash), "hash collision");
Copy link
Contributor

Choose a reason for hiding this comment

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

This check also doesn't prevent misbehavior. Can be on the client.

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.

6 participants