Skip to content
This repository was archived by the owner on Mar 11, 2025. It is now read-only.

Remove unconditional hidden commitment override for sanity #1528

Merged
merged 1 commit into from
Mar 30, 2021

Conversation

ryoqun
Copy link
Contributor

@ryoqun ryoqun commented Mar 30, 2021

Currently, sendTransaction via spl-token is confirmed with recent, even if the connection's confirmation is confirmed for example.
This means any token transaction async executions could return too early, causing very hard-to-debug issues:

The following harmless-looking code doesn't work for some time:

        try {
          await this.createAssociatedTokenAccountInternal(     <=  this submits tx and it's confirmed with recent internally
            owner,
            associatedAddress,
          );
        } catch (err) {
          // ignore all errors; for now there is no API compatible way to
          // selectively ignore the expected instruction error if the
          // associated account is existing already.
        }

        // Now this should always succeed
        return await this.getAccountInfo(associatedAddress); <= this is fetched with confirmed internally; could return pre-the-tx state.

Also, actually, we're seeing very confusing errors on the wild.

Last and not least, our partners are silently using recent for tx confirmation, this is too dangerous imo.

solution

just use connection's configured commitment value.

I looked around git history maybe solana-labs/solana#12498 is the reason of this? How about doing recent for example?

could possibly affecting solana-labs/solana#15705

Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense, great catch! The earlier issue was related to some instability in the localnet validator on 1.2 or 1.3, so I can close it.

@ryoqun ryoqun merged commit 9621595 into solana-labs:master Mar 30, 2021
@ryoqun
Copy link
Contributor Author

ryoqun commented Mar 30, 2021

@joncinque thanks for reviewing!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants