eth/client: Redeem #1302
eth/client: Redeem #1302
Conversation
martonp
force-pushed
the
ethRedeem
branch
3 times, most recently
from
06ecb03
to
8533e90
Compare
client/asset/eth/eth_test.go
Outdated
| if n.redeemErr != nil { | ||
| return nil, n.redeemErr | ||
| } | ||
| /*baseTx := &types.DynamicFeeTx{ |
There was a problem hiding this comment.
Did you want to leave these comments?
There was a problem hiding this comment.
No.. I'll take them out of initiate also.
| // redeem redeems a swap contract. Any on-chain failure, such as this secret not | ||
| // matching the hash, will not cause this to error. |
There was a problem hiding this comment.
such as this secret not matching the hash
We should check this in Redeem though.
| FeeSuggestion uint64 | ||
| // AssetVersion is the swap protocol version, which may indicate a specific | ||
| // contract or form of contract. | ||
| AssetVersion uint32 |
There was a problem hiding this comment.
Pretty sure I'll clean it out in 1320, merging with this solution now
|
I got that too but it went away after another run.. I think it has to do with the issue with a fresh harness. |
| } | ||
| outputCoin := eth.createAmountCoin(redeemedValue) | ||
| fundsRequired := dexeth.RedeemGas(len(form.Redemptions), form.AssetVersion) * form.FeeSuggestion | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes this was the whole point haha, I've updated it
There was a problem hiding this comment.
It does seem like the first round of simnet tests always fails recently... #1327
| if err != nil { | ||
| return fail(fmt.Errorf("Redeem: redeem error: %w", err)) | ||
| } |
There was a problem hiding this comment.
Should a failure here unlock the coins?
There was a problem hiding this comment.
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.
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.
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.
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.
Are these locked? I see fundsRequired saved to t.metaData.RedemptionFeesPaid in trade.go, but I don't see any locking or unlocking.
There was a problem hiding this comment.
#1289 (comment)
EDIT: bad link initially. Goes to a discussion with @martonp on that PR
There was a problem hiding this comment.
We can figure it out there then.
| @@ -145,6 +145,7 @@ type ethFetcher interface { | |||
| initiate(ctx context.Context, contracts []*asset.Contract, maxFeeRate uint64, contractVer uint32) (*types.Transaction, error) | |||
| shutdown() | |||
| syncProgress() ethereum.SyncProgress | |||
| isRedeemable(secretHash, secret [32]byte, contractVer uint32) (bool, error) | |||
There was a problem hiding this comment.
This is good for me, but I have a suspicious that redeem can call the contractor method of isRedeemable internally.
Part of #1154