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: Add Ethereum server asset. #979

Merged
merged 8 commits into from May 2, 2021

Conversation

JoeGruffins
Copy link
Member

Work towards #939
Depends upon #956 and #944

Adds a skeleton eth server client.

@JoeGruffins JoeGruffins force-pushed the dexethserverclient branch 2 times, most recently from 451bd67 to 515979d Compare February 18, 2021 06:38
@JoeGruffins JoeGruffins force-pushed the dexethserverclient branch 2 times, most recently from 0c433e8 to 666c83a Compare March 2, 2021 08:32
$CHAIN_PASSWORD
$ADDRESS_PASSWORD
Copy link
Member Author

Choose a reason for hiding this comment

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

It does not appear that this was unlocking the account. We will pass the password when sending funds.

Comment on lines 345 to 373
// decodeCoinID decodes the coin ID into a contract address and secret hash.
func decodeCoinID(coinID []byte) (common.Address, []byte, error) {
if len(coinID) != coinIDSize {
return common.Address{}, nil, fmt.Errorf("coin ID wrong length. expected %d, got %d",
coinIDSize, len(coinID))
}
secretHash := make([]byte, 32)
copy(secretHash, coinID[20:])
return common.BytesToAddress(coinID[:20]), secretHash, nil
}
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 believe a "coin" is best identified by the address of the contract it belongs to and the secret hash it is keyed by. This is tentative and up for debate. See #1001
All info about the eth side of a swap will be accessed through the contract.

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't the eth contract address be a constant?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes. If we update the contract, though, I assume it would change over time.

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 you're right.

I'm hesitant because it's hard to change a coin ID encoding once it's used on mainnet (unless we add a version byte?). I would hope that the contract would not change, even to the point that different servers would use the same contract by default. But in actuality, we might need to change it someday, and if we didn't encode it, we wouldn't necessarily have the information we need from the coin ID.

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'm not sure I follow that last part of that. There would not be any need to change a coin ID. Client would initiate the swap with a contract. Server will have a map of contract addresses and reject older contracts if necessary with a penalty. It makes perfect sense that all servers will be using the same contract. I don't think that is a problem.

I believe contracts will need to be updated for security reasons, and if eth consensus rules change.

Copy link
Member

Choose a reason for hiding this comment

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

There would not be any need to change a coin ID

Sorry. Not an individual coin ID. The coin ID encoding protocol. It's hard to change it later.

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 think I will add a byte for flags. At least we might want unique ids for maker and taker, otherwise they would be the same. If there's somewhere using coin ids as a map key, expecting them to be unique... A one bit flag in the byte field could resolve this, and store a limited amount of arbitrary data in the future. maybe make it two bytes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added flags anyway. The scheme may be different in the end, as long as we decide on something before allowing mainnet. It may seem like this doesn't allow direct payments for a fee, but, a contract may be the best way to handle a fee payment. We could also have a flag that designates this is a transaction hash, then the "secret hash" could instead be the txid in that case.

Copy link
Member

Choose a reason for hiding this comment

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

I was wondering about normal transactions too, such as withdraw/deposit and how those would be encoded. I suspect we will need some kind of dynamic encoding with the flags to switch it to tx hash as you suggested. I'm just catching up on your eth work though so take that with a grain of salt.

Copy link
Member

Choose a reason for hiding this comment

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

Thinking through the coin ID issue: #1021

@JoeGruffins JoeGruffins marked this pull request as ready for review March 5, 2021 09:33
Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

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

This is a great start to the server-side work. Having this skeleton in place will help us to visualize and compartmentalize the work for efficient development.

After the create-node.sh fix, this tests well. This will all be iterated over as we get deeper into implementation, so I think we can get this in soon.

@chappjc, any interest in maintaining a separate eth integration branch?

dex/testing/eth/create-node.sh Outdated Show resolved Hide resolved
Comment on lines 345 to 373
// decodeCoinID decodes the coin ID into a contract address and secret hash.
func decodeCoinID(coinID []byte) (common.Address, []byte, error) {
if len(coinID) != coinIDSize {
return common.Address{}, nil, fmt.Errorf("coin ID wrong length. expected %d, got %d",
coinIDSize, len(coinID))
}
secretHash := make([]byte, 32)
copy(secretHash, coinID[20:])
return common.BytesToAddress(coinID[:20]), secretHash, nil
}
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 you're right.

I'm hesitant because it's hard to change a coin ID encoding once it's used on mainnet (unless we add a version byte?). I would hope that the contract would not change, even to the point that different servers would use the same contract by default. But in actuality, we might need to change it someday, and if we didn't encode it, we wouldn't necessarily have the information we need from the coin ID.

dex/testing/eth/create-node.sh Show resolved Hide resolved
server/asset/eth/client_harness_test.go Outdated Show resolved Hide resolved
@chappjc
Copy link
Member

chappjc commented Mar 10, 2021

@chappjc, any interest in maintaining a separate eth integration branch?

We can if it seems like eth work has the potential to break other things, but I don't mind having incomplete features on master if everything else is functional.

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.

Good to go with a go.sum resolve please (although either wisdom or yourself will have to resolve twice depending on who goes first).

I'm pretty confident we will have to evolve the coin ID encoding for ethereum, sometimes holding transactions and sometimes accounts and other data like secret hash as you have here, but we can definitely go forward with this.

Switch full nodes to fast nodes, which is most likely what servers will
run, and add two light nodes, which is most likely what clients will
run.
@chappjc chappjc merged commit 49dc413 into decred:master May 2, 2021
@chappjc chappjc added the ETH label Jul 20, 2021
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.

None yet

3 participants