-
Notifications
You must be signed in to change notification settings - Fork 92
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/client: Redeem #1302
eth/client: Redeem #1302
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
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 | ||
|
@@ -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 | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is where we should be calling the free public view There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should a failure here unlock the coins? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are these locked? I see There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #1289 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, ok. Thank you. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We should check this in |
||
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) | ||
|
There was a problem hiding this comment.
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 ofisRedeemable
internally.