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.

Sign up for GitHub

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

Jump to bottom

eth/client: Redeem #1302

Merged
merged 4 commits into from
Dec 7, 2021
Merged

eth/client: Redeem #1302

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3763b86
eth/client: Redeem
martonp Dec 3, 2021
18a6d3a
Updates based on review.
martonp Dec 3, 2021
0e5bfbd
Use isRedeemable to check if redeem is possible
martonp Dec 7, 2021
870408f
Update harness test
martonp Dec 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
47 changes: 44 additions & 3 deletions client/asset/eth/eth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,8 @@ type ethFetcher interface {
initiate(ctx context.Context, contracts []*asset.Contract, maxFeeRate uint64, contractVer uint32) (*types.Transaction, error)
shutdown()
syncProgress() ethereum.SyncProgress
redeem(txOpts *bind.TransactOpts, redemptions []*asset.Redemption, contractVer uint32) (*types.Transaction, error)
isRedeemable(secretHash, secret [32]byte, contractVer uint32) (bool, error)
Copy link
Member

Choose a reason for hiding this comment

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

This is good for me, but I have a suspicious that redeem can call the contractor method of isRedeemable internally.

redeem(ctx context.Context, redemptions []*asset.Redemption, maxFeeRate uint64, contractVer uint32) (*types.Transaction, error)
refund(txOpts *bind.TransactOpts, secretHash [32]byte, contractVer uint32) (*types.Transaction, error)
swap(ctx context.Context, secretHash [32]byte, contractVer uint32) (*dexeth.SwapState, error)
lock() error
Expand Down Expand Up @@ -672,8 +673,48 @@ func (eth *ExchangeWallet) Swap(swaps *asset.Swaps) ([]asset.Receipt, asset.Coin

// Redeem sends the redemption transaction, which may contain more than one
// redemption.
func (*ExchangeWallet) Redeem(form *asset.RedeemForm) ([]dex.Bytes, asset.Coin, uint64, error) {
return nil, nil, 0, asset.ErrNotImplemented
func (eth *ExchangeWallet) Redeem(form *asset.RedeemForm) ([]dex.Bytes, asset.Coin, uint64, error) {
fail := func(err error) ([]dex.Bytes, asset.Coin, uint64, error) {
return nil, nil, 0, err
}

if len(form.Redemptions) == 0 {
return fail(errors.New("Redeem: must be called with at least 1 redemption"))
}

inputs := make([]dex.Bytes, 0, len(form.Redemptions))
var redeemedValue uint64
for _, redemption := range form.Redemptions {
var secretHash, secret [32]byte
copy(secretHash[:], redemption.Spends.SecretHash)
copy(secret[:], redemption.Secret)
redeemable, err := eth.node.isRedeemable(secretHash, secret, form.AssetVersion)
if err != nil {
return fail(fmt.Errorf("Redeem: failed to check if swap is redeemable: %w", err))
}
if !redeemable {
return fail(fmt.Errorf("Redeem: secretHash %x not redeemable with secret %x",
secretHash, secret))
}

swapData, err := eth.node.swap(eth.ctx, secretHash, form.AssetVersion)
if err != nil {
return nil, nil, 0, fmt.Errorf("Redeem: error finding swap state: %w", err)
}
redeemedValue += swapData.Value
inputs = append(inputs, redemption.Spends.Coin.ID())
}
outputCoin := eth.createAmountCoin(redeemedValue)
fundsRequired := dexeth.RedeemGas(len(form.Redemptions), form.AssetVersion) * form.FeeSuggestion

Copy link
Member

@chappjc chappjc Dec 5, 2021 •
edited

Choose a reason for hiding this comment

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

This is where we should be calling the free public view isRedeemable, I believe. The next step is to broadcast any number of secrets, which can be used to immediately redeem assets on another chain, so the txn better not fail on execution on a remote node.
The call is free, so we should do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this was the whole point haha, I've updated it

// TODO: make sure the amount we locked for redemption is enough to cover the gas
// fees. Also unlock coins.
_, err := eth.node.redeem(eth.ctx, form.Redemptions, form.FeeSuggestion, form.AssetVersion)
if err != nil {
return fail(fmt.Errorf("Redeem: redeem error: %w", err))
}
Comment on lines +713 to +715
Copy link
Member

Choose a reason for hiding this comment

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

Should a failure here unlock the coins?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A success should unlock the coins as well. It's actually not so easy to handle the failure, since failure will actually consume some gas as well.. and if it keeps failing then we may use more gas than we had locked in the first place. Anyways, I have a TODO in there regarding this and there will be another PR regarding locking/unlocking funds for redemption, so this can be handled there.

Copy link
Member

Choose a reason for hiding this comment

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

If the error is a communication error I don't think the fee would be lost. I'm not sure if their could be another reason for a failure here. If the redeem fails on-chain I don't think there will be an error.

A TODO is fine for now imo. Will be easier to diagnose when we start actually trading with eth.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah you're right, on chain error doesn't fail there. In that case we don't need to unlock funds for failure since it will be retried anyways.

Copy link
Member

Choose a reason for hiding this comment

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

It could be a communication error though with the internal geth node and the tx is never sent over the wire. I think that's the only type of error possible, but not sure. In that case the redeem gas stays locked. Doesn't it? I could be wrong.

Copy link
Member

Choose a reason for hiding this comment

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

Are these locked? I see fundsRequired saved to t.metaData.RedemptionFeesPaid in trade.go, but I don't see any locking or unlocking.

Copy link
Member

@chappjc chappjc Dec 7, 2021 •
edited

Choose a reason for hiding this comment

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

#1289 (comment)
EDIT: bad link initially. Goes to a discussion with @martonp on that PR

Copy link
Member

Choose a reason for hiding this comment

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

#1314

Copy link
Member

Choose a reason for hiding this comment

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

Oh, ok. Thank you.

Copy link
Member

Choose a reason for hiding this comment

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

We can figure it out there then.


return inputs, outputCoin, fundsRequired, nil
}

// SignMessage signs the message with the private key associated with the
Expand Down
271 changes: 253 additions & 18 deletions client/asset/eth/eth_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,10 @@ type testNode struct {
swapMap map[[32]byte]*dexeth.SwapState
swapErr error
initErr error
redeemErr error
nonce uint64
redeemable bool
isRedeemableErr error
}

func newBalance(current, in, out uint64) *Balance {
Expand Down Expand Up @@ -113,30 +116,25 @@ func (n *testNode) initiate(ctx context.Context, contracts []*asset.Contract, ma
if n.initErr != nil {
return nil, n.initErr
}
// baseTx := &types.DynamicFeeTx{
// Nonce: n.nonce,
// GasFeeCap: maxFeeRate,
// GasTipCap: MinGasTipCap,
// Gas: dexeth.InitGas(len(contracts)),
// Value: opts.Value,
// Data: []byte{},
// }
tx = types.NewTx(&types.DynamicFeeTx{})
// n.nonce++
// n.lastInitiation = initTx{
// initiations: initiations,
// hash: tx.Hash(),
// opts: opts,
// }
n.nonce++
return types.NewTx(&types.DynamicFeeTx{
Nonce: n.nonce,
}), nil
}
func (n *testNode) redeem(opts *bind.TransactOpts, redemptions []*asset.Redemption, contractVer uint32) (*types.Transaction, error) {
return nil, nil
func (n *testNode) isRedeemable(secretHash [32]byte, secret [32]byte, contractVer uint32) (redeemable bool, err error) {
return n.redeemable, n.isRedeemableErr
}
func (n *testNode) redeem(ctx context.Context, redemptions []*asset.Redemption, maxFeeRate uint64, contractVer uint32) (*types.Transaction, error) {
if n.redeemErr != nil {
return nil, n.redeemErr
}
n.nonce++
return types.NewTx(&types.DynamicFeeTx{
Nonce: n.nonce,
}), nil
}
func (n *testNode) refund(opts *bind.TransactOpts, secretHash [32]byte, serverVer uint32) (*types.Transaction, error) {
func (n *testNode) refund(txOpts *bind.TransactOpts, secretHash [32]byte, contractVer uint32) (*types.Transaction, error) {
return nil, nil
}
func (n *testNode) swap(ctx context.Context, secretHash [32]byte, contractVer uint32) (*dexeth.SwapState, error) {
Expand All @@ -153,7 +151,6 @@ func (n *testNode) swap(ctx context.Context, secretHash [32]byte, contractVer ui
}
return swap, nil
}

func (n *testNode) signData(addr common.Address, data []byte) ([]byte, error) {
if n.signDataErr != nil {
return nil, n.signDataErr
Expand Down Expand Up @@ -1137,6 +1134,244 @@ func TestPreRedeem(t *testing.T) {
}
}

func TestRedeem(t *testing.T) {
node := &testNode{
swapVers: map[uint32]struct{}{
0: {},
},
swapMap: make(map[[32]byte]*dexeth.SwapState),
}
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
eth := &ExchangeWallet{
node: node,
ctx: ctx,
log: tLogger,
}
addSwapToSwapMap := func(secretHash [32]byte, value uint64, step dexeth.SwapStep) {
swap := dexeth.SwapState{
BlockHeight: 1,
LockTime: time.Now(),
Initiator: common.HexToAddress("0x2b84C791b79Ee37De042AD2ffF1A253c3ce9bc27"),
Participant: common.HexToAddress("B6De8BB5ed28E6bE6d671975cad20C03931bE981"),
Value: value,
State: step,
}
node.swapMap[secretHash] = &swap
}

numSecrets := 3
secrets := make([][32]byte, 0, numSecrets)
secretHashes := make([][32]byte, 0, numSecrets)
for i := 0; i < numSecrets; i++ {
var secret [32]byte
copy(secret[:], encode.RandomBytes(32))
secretHash := sha256.Sum256(secret[:])
secrets = append(secrets, secret)
secretHashes = append(secretHashes, secretHash)
}

addSwapToSwapMap(secretHashes[0], 1e9, dexeth.SSInitiated)
addSwapToSwapMap(secretHashes[1], 1e9, dexeth.SSInitiated)

tests := []struct {
name string
form asset.RedeemForm
redeemErr error
isRedeemable bool
isRedeemableErr error
expectError bool
}{
{
name: "ok",
expectError: false,
isRedeemable: true,
form: asset.RedeemForm{
Redemptions: []*asset.Redemption{
{
Spends: &asset.AuditInfo{
SecretHash: secretHashes[0][:],
Coin: &coin{
id: encode.RandomBytes(32),
},
},
Secret: secrets[0][:],
},
{
Spends: &asset.AuditInfo{
SecretHash: secretHashes[1][:],
Coin: &coin{
id: encode.RandomBytes(32),
},
},
Secret: secrets[1][:],
},
},
FeeSuggestion: 100,
AssetVersion: 0,
},
},
{
name: "not redeemable",
expectError: true,
isRedeemable: false,
form: asset.RedeemForm{
Redemptions: []*asset.Redemption{
{
Spends: &asset.AuditInfo{
SecretHash: secretHashes[0][:],
Coin: &coin{
id: encode.RandomBytes(32),
},
},
Secret: secrets[0][:],
},
{
Spends: &asset.AuditInfo{
SecretHash: secretHashes[1][:],
Coin: &coin{
id: encode.RandomBytes(32),
},
},
Secret: secrets[1][:],
},
},
FeeSuggestion: 100,
AssetVersion: 0,
},
},
{
name: "isRedeemable error",
expectError: true,
isRedeemable: true,
isRedeemableErr: errors.New(""),
form: asset.RedeemForm{
Redemptions: []*asset.Redemption{
{
Spends: &asset.AuditInfo{
SecretHash: secretHashes[0][:],
Coin: &coin{
id: encode.RandomBytes(32),
},
},
Secret: secrets[0][:],
},
{
Spends: &asset.AuditInfo{
SecretHash: secretHashes[1][:],
Coin: &coin{
id: encode.RandomBytes(32),
},
},
Secret: secrets[1][:],
},
},
FeeSuggestion: 100,
AssetVersion: 0,
},
},
{
name: "redeem error",
redeemErr: errors.New(""),
isRedeemable: true,
expectError: true,
form: asset.RedeemForm{
Redemptions: []*asset.Redemption{
{
Spends: &asset.AuditInfo{
SecretHash: secretHashes[0][:],
Coin: &coin{
id: encode.RandomBytes(32),
},
},
Secret: secrets[0][:],
},
},
FeeSuggestion: 200,
AssetVersion: 0,
},
},
{
name: "swap not found in contract",
isRedeemable: true,
expectError: true,
form: asset.RedeemForm{
Redemptions: []*asset.Redemption{
{
Spends: &asset.AuditInfo{
SecretHash: secretHashes[2][:],
Coin: &coin{
id: encode.RandomBytes(32),
},
},
Secret: secrets[2][:],
},
},
FeeSuggestion: 100,
AssetVersion: 0,
},
},
{
name: "empty redemptions slice error",
isRedeemable: true,
expectError: true,
form: asset.RedeemForm{
Redemptions: []*asset.Redemption{},
FeeSuggestion: 100,
AssetVersion: 0,
},
},
}

for _, test := range tests {
node.redeemErr = test.redeemErr
node.redeemable = test.isRedeemable
node.isRedeemableErr = test.isRedeemableErr

ins, out, fees, err := eth.Redeem(&test.form)
if test.expectError {
if err == nil {
t.Fatalf("%v: expected error", test.name)
}
continue
}
if err != nil {
t.Fatalf("%v: unexpected error: %v", test.name, err)
}

if len(ins) != len(test.form.Redemptions) {
t.Fatalf("%v: expected %d inputs but got %d",
test.name, len(test.form.Redemptions), len(ins))
}

// Check fees returned from Redeem are as expected
expectedGas := dexeth.RedeemGas(len(test.form.Redemptions), 0)
expectedFees := expectedGas * test.form.FeeSuggestion
if fees != expectedFees {
t.Fatalf("%v: expected fees %d, but got %d", test.name, expectedFees, fees)
}

var totalSwapValue uint64
for i, redemption := range test.form.Redemptions {
coinID := redemption.Spends.Coin.ID()
if !bytes.Equal(coinID, ins[i]) {
t.Fatalf("%v: expected input %x to equal coin id %x",
test.name, coinID, ins[i])
}

var secretHash [32]byte
copy(secretHash[:], redemption.Spends.SecretHash)
swap := node.swapMap[secretHash]
totalSwapValue += swap.Value
}

if out.Value() != totalSwapValue {
t.Fatalf("expected coin value to be %d but got %d",
totalSwapValue, out.Value())
}
}
}

func TestMaxOrder(t *testing.T) {
gases := dexeth.VersionedGases[0]

Expand Down
13 changes: 7 additions & 6 deletions client/asset/eth/nodeclient.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,7 @@ func (n *nodeClient) addSignerToOpts(txOpts *bind.TransactOpts) error {

// initiate initiates multiple swaps in the same transaction.
func (n *nodeClient) initiate(ctx context.Context, contracts []*asset.Contract, maxFeeRate uint64, contractVer uint32) (tx *types.Transaction, err error) {
gas := dexeth.InitGas(len(contracts), 0)
gas := dexeth.InitGas(len(contracts), contractVer)
var val uint64
for _, c := range contracts {
val += c.Value
Expand Down Expand Up @@ -401,12 +401,13 @@ func (n *nodeClient) estimateRefundGas(ctx context.Context, secretHash [32]byte,
})
}

// redeem redeems a swap contract. The redeemer will be the account at txOpts.From.
// Any on-chain failure, such as this secret not matching the hash, will not cause
// this to error.
func (n *nodeClient) redeem(txOpts *bind.TransactOpts, redemptions []*asset.Redemption, contractVer uint32) (tx *types.Transaction, err error) {
// redeem redeems a swap contract. Any on-chain failure, such as this secret not
// matching the hash, will not cause this to error.
Comment on lines +404 to +405
Copy link
Member

Choose a reason for hiding this comment

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

such as this secret not matching the hash

We should check this in Redeem though.

func (n *nodeClient) redeem(ctx context.Context, redemptions []*asset.Redemption, maxFeeRate uint64, contractVer uint32) (tx *types.Transaction, err error) {
gas := dexeth.RedeemGas(len(redemptions), contractVer)
txOpts, _ := n.txOpts(ctx, 0, gas, dexeth.GweiToWei(maxFeeRate))
if err := n.addSignerToOpts(txOpts); err != nil {
return nil, err
return nil, fmt.Errorf("addSignerToOpts error: %w", err)
}
return tx, n.withcontractor(contractVer, func(c contractor) error {
tx, err = c.redeem(txOpts, redemptions)
Expand Down