-
Notifications
You must be signed in to change notification settings - Fork 91
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
client/asset/eth: SwapConfirmations #1315
Conversation
Add asset version argument.
client/core/trade.go
Outdated
@@ -1064,7 +1064,7 @@ func (t *trackedTrade) shouldBeginFindRedemption(ctx context.Context, match *mat | |||
return false | |||
} | |||
|
|||
confs, spent, err := t.wallets.fromWallet.SwapConfirmations(ctx, swapCoinID, proof.Script, match.MetaData.Stamp) | |||
confs, spent, err := t.wallets.fromWallet.SwapConfirmations(ctx, swapCoinID, proof.Script, match.MetaData.Stamp, t.metaData.FromVersion) |
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.
Presently eth's (*swapReceipt).Contract() dex.Bytes
returns a slice that contains the secret hash, and this is shoe-horned into MatchProof.Script
.
What if it returned a slice that was a concatenation of contract version and secret hash, so that client.asset.Receipt.Contract()
, which is assigned to proof.Script
, has this info. It's already not a script at all as implied by the MatchProof.Script
field name, and it's intended to be an opaque blob for multi-asset support anyway, so why not have it encode the version too?
That is, we're already freely reinterpreting the Receipt.Contract
method, so I'd just assume we use it to convey all the info needed about this contract-swap rather than add an arg to SwapConfirmations
, which without close examination is kind of a head-scratcher and in fact of no use to existing assets.
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.
Nice. Done.
} | ||
|
||
spent = swapData.State >= dexeth.SSRedeemed | ||
confs = uint32(hdr.Number.Uint64() - swapData.BlockHeight + 1) |
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.
Looking good in this method, and it does seem to be alright if the contract dex.Bytes
arg encoded both the asset version and the secret hash. Open to that?
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.
i.e. concept: bbc9d81
func (eth *ExchangeWallet) SwapConfirmations(ctx context.Context, _ dex.Bytes, contractData dex.Bytes,
_ time.Time) (confs uint32, spent bool, err error) {
if len(contractData) != 36 {
return 0, false, fmt.Errorf("invalid contract data")
}
// contractData fully specifies this swap in terms of which contract version
// is used and the secret hash that keys the unique swap.
assetVersion := binary.BigEndian.Uint32(contractData[:4])
var secretHash [32]byte
copy(secretHash[:], contractData[4:])
swapData, err := eth.node.swap(ctx, secretHash, assetVersion)
if err != nil {
return 0, false, fmt.Errorf("error finding swap state: %w", err)
}
if swapData.State == dexeth.SSNone {
return 0, false, asset.CoinNotFoundError
}
hdr, err := eth.node.bestHeader(ctx)
if err != nil {
return 0, false, fmt.Errorf("error fetching best header: %w", err)
}
spent = swapData.State >= dexeth.SSRedeemed
confs = uint32(hdr.Number.Uint64() - swapData.BlockHeight + 1)
return
}
83d3910
to
a2816d1
Compare
Add an asset version argument to
(Wallet).SwapConfirmations
and implement the method for eth.