In D16821#381670, @bytesofman wrote:

So i this case -- we are using locktime because it's an existing field that is available in script. But we are not really setting a locktime -- we are setting a sort of nonce, that should be a unique ID.

That’s indeed the purpose, exactly

per inline comment above -- from the app dev perspective, I don't think we should call it locktime. Can mention in comments / docs that it's important to assign an unique ID and that this is done with locktime.

Well, technically this also can be used to create future offers, offers that are valid but only will come into effect at a future date. And to be a nonce, it has to fall into a specific range of values. If people just set this to a random integer it will likely fall into a future timestamp.

Past that -- this solution is certainly better than having nothing for this case -- I can see the case of "same qty of tokens at same price" coming up a lot. However -- I don't know if this is a good practical solution for the problem.

Since these txs are all public on the blockchain -- could someone target offers?

An app dev can be expected to figure out locktime values that are different nonces. But what if someone just wants to watch the dex burn. Could an attacker look for open offers, then copy them -- including the same locktime?

Yes but keep in mind that everything has to be identical, including the makerPk. So the “victim” will get paid either way, and they wouldn’t even notice anything happened.

If this happens -- is it just the attackers tokens that get burned? If that's the case....well, NBD, we can't stop people from burning tokens and we don't want to. But if this sort of "attack" could break the available sell listing in some way, we might need a different approach.

Yes, only the attacker‘s tokens would be burned. We need to protect against normal users getting their tokens burned by an attacker, not prevent “attackers” from burning their own tokens.

So this change mitigates this issue—users who create multiple identical offers with different lock times will be protected, but people who don’t do so will be vulnerable to having their tokens burned. That includes “attackers” that just burn their own tokens.

But I think what we should do it also add an object that safely creates good lock times (and storing previous one internally), and then people just forward this value from that object. Would also allow for future extensions.

But better to do this in a separate diff.

keep in mind that everything has to be identical, including the makerPk.

ah ok yeah, so nbd. an attacker COULD make this kind of offer, but then the original offerer could cancel it for free tokens, or otherwise see weird chain behavior. but it's not like we can stop "attackers" from just burning tokens they want to burn anyway.

also add an object that safely creates good lock times (and storing previous one internally)

not sure how complicated we want to make this. depends what this looks like. it does sound like a good idea but also we don't want to be adding a database for this.

Well, technically this also can be used to create future offers, offers that are valid but only will come into effect at a future date.

seems like people have been saying this about locktime for the better part of a decade now but I don't think I've ever seen this actually come up. anyway, as long as it's documented that this is a "special case" and not actually locktime...I mean at this point, an app dev working in this lib has to be pretty familiar with the chain and comfortable with a good deal of crypto complexity.

And to be a nonce, it has to fall into a specific range of values. If people just set this to a random integer it will likely fall into a future timestamp.

what happens next? in this case, it's only valid in the future?

mb worth a test, or an error if the user sets this?

In D16821#382053, @bytesofman wrote:

really does get us into some interesting complexity needing to check the existing orderbook before making an order.

Yes, but I tried to encapsulate it as much as possible. And it also just queries one specific script hash, not the entire orderbook.

the main thing is that we are sure any loss is confined to the person creating the duplicate order, and would be avoided by correct use of the tool.

Yeah exactly. And the way it’s designed is that someone misusing it will stumble over enforcedLockTime, and either be reckless and just put something there, or read the docs and use it correctly. So this should limit misuse.

imo good enough for a v1. in practice, might see a token listing power user elect to use large amounts of "one shot" offers instead of partials. can't really tell without building out more dex tools and seeing this stuff in action.

Note (important): We should actually discourage people from using oneshot offers for fungible tokens, as it has the same issue as the partial offers (actually this would be inherent in every kind of covenant, even if we had introspection etc.).

The reason why I’m not rushing to fix oneshot offers is because it’s intended to be used only for NFTs, and those by necessity must be unique anyway.

So we should at least add a disclaimer or add a ONESHOT2 version.