fix(web): enforce sequential transactions

[thehowl]

Should avoid once and for all "not initialized" issues and "invalid signature" issues.

[netlify]

✅ Deploy Preview for gnochess-signup-form canceled.

Name	Link
🔨 Latest commit	21b8358
🔍 Latest deploy log	https://app.netlify.com/sites/gnochess-signup-form/deploys/651322e0b96e3500080648a8

[netlify]

✅ Deploy Preview for gnochess ready!

Name	Link
🔨 Latest commit	21b8358
🔍 Latest deploy log	https://app.netlify.com/sites/gnochess/deploys/651322e0b984860008fe436a
😎 Deploy Preview	https://deploy-preview-157--gnochess.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[clockworkgr]

I don't really like this tbh. The user TXs should be sequential by the game logic alone. This essentially covers up the fact we have a logic error somewhere

[clockworkgr]

Also, the use of finally means we re proceeding with whatever logic even if a previous TX has failed rather than cleaning up/attempting to recover

[thehowl]

I don't really like this tbh. The user TXs should be sequential by the game logic alone. This essentially covers up the fact we have a logic error somewhere

Requests are slow and concurrent requests may happen, without logic to prevent it. Even just by a user clicking things too fast. (Ie, moving and then resigning)

Also, the use of finally means we re proceeding with whatever logic even if a previous TX has failed rather than cleaning up/attempting to recover

I used finally just so transactions didn't stop processing just because we have a failing one. I don't necessarily know if having a finally callback has side-effects on async functions, but in my mind it shouldn't be blocking any exceptions to be thrown (and eventually captured) down the line?

[clockworkgr]

I don't really like this tbh. The user TXs should be sequential by the game logic alone. This essentially covers up the fact we have a logic error somewhere

Requests are slow and concurrent requests may happen, without logic to prevent it. Even just by a user clicking things too fast. (Ie, moving and then resigning)

we're using commit broadcasts and in theory they should be properly awaited....with JS being single-threaded a TX should NEVER be broadcasted until the previous one has committed/returned....if it does, we've messed up somewhere (hence my point about covering up logic errors)... the quick move -> resign flow does have me thinking however....Still think we should address that with UI logic rather than the proposed solution in this PR however

Also, the use of finally means we re proceeding with whatever logic even if a previous TX has failed rather than cleaning up/attempting to recover

I used finally just so transactions didn't stop processing just because we have a failing one. I don't necessarily know if having a finally callback has side-effects on async functions, but in my mind it shouldn't be blocking any exceptions to be thrown (and eventually captured) down the line?

Not a question of side effects and you are correct in your assumptions. My point is that is something has messed up and we need to recover from it in the catch, we probably don't want to continue/send any more txs until we've sorted it out

[thehowl]

we're using commit broadcasts and in theory they should be properly awaited....with JS being single-threaded a TX should NEVER be broadcasted until the previous one has committed/returned....

if you're saying that this should be the responsibility of the library to do it, you're right. though I think it's something that can be reported and fixed separately, while we have this fix here in the meantime.

also, I don't think js being single threaded is at all relevant, as requests are still async and as such a new req can happen while the other one is still inflight, and in fact i think is very much what happens here.

Not a question of side effects and you are correct in your assumptions. My point is that is something has messed up and we need to recover from it in the catch, we probably don't want to continue/send any more txs until we've sorted it out

mhh, it's not the responsibility of callMethod to handle the error. we have error handlers elsewhere already. also, errors don't mean something extraordinarily wrong in this context; they can be extremely mundane, and just a panic from the realm to rollback the TX.

[clockworkgr]

we're using commit broadcasts and in theory they should be properly awaited....with JS being single-threaded a TX should NEVER be broadcasted until the previous one has committed/returned....

if you're saying that this should be the responsibility of the library to do it, you're right. though I think it's something that can be reported and fixed separately, while we have this fix here in the meantime.

also, I don't think js being single threaded is at all relevant, as requests are still async and as such a new req can happen while the other one is still inflight, and in fact i think is very much what happens here.

it's not really relevant in this case, I was mostly pointing to the fact that you ONLY have to deal with logic issues (e.g. disable resign button when make move tx is in flight) rather than potential concurrency weirdness

Not a question of side effects and you are correct in your assumptions. My point is that is something has messed up and we need to recover from it in the catch, we probably don't want to continue/send any more txs until we've sorted it out

mhh, it's not the responsibility of callMethod to handle the error. we have error handlers elsewhere already. also, errors don't mean something extraordinarily wrong in this context; they can be extremely mundane, and just a panic from the realm to rollback the TX.

precisely my point , it can be something mundane and recoverable. doesn't mean the following tx must be broadcasted if the current oen has failed though before we figure out what's wrong (technically, if the UI logic is sound we'd never queue the following tx if the current one fails but in that case this PR is not necessary)

Anyway...as a quick fix it's fine , it shouldnt stop us from digging out any minor logic mistakes though

[zivkovicmilos]

if you're saying that this should be the responsibility of the library to do it, you're right. though I think it's something that can be reported and fixed separately, while we have this fix here in the meantime.

I would like to note that absolutely no blockchain client library does this, ever; from Ethereum to Cosmos. The only reason we are using commit broadcasts in the first place, is because we don't have fundamental support (yet, or ever) in Gno for indexing transactions. It is the sole responsibility of the caller to manage their transactions, and having any library "buffer" calls is not functionality I would ever consider adding into our js / go libs.

As @clockworkgr mentioned, the commit send is almost never used in real production environments because it blocks the calling context until the transactions are committed to the chain. We could easily use the sync broadcast (as we should), and wait for the transaction to be committed, at any point in time, asynchronously. However, due to a lack of indexing support, this is not an optimal way to go, even if the client lib provides transaction waits (simulates indexing for the caller).

The general philosophy of Cosmos-based chains is if the transaction dies, for whatever reason, it dies - and it is the responsibility of the caller to re-execute it again. One such calling context is in supernova, here, that manages the complete flow of transactions from creation to commitment (and it uses sync sends, with recovery).

I strongly agree with @clockworkgr on the fact that we wouldn't be having this conversation if we had much tighter control over how transactions are being sent out (and in which order) in the game. This is the only way to guarantee "synchronicity" of transactions for a specific account