Skip to content

feat: Add tokio support to anchor_client RequestBuilder#3057

Merged
acheroncrypto merged 12 commits into
otter-sec:masterfrom
cryptopapi997:tokio-anchor-client
Jul 12, 2024
Merged

feat: Add tokio support to anchor_client RequestBuilder#3057
acheroncrypto merged 12 commits into
otter-sec:masterfrom
cryptopapi997:tokio-anchor-client

Conversation

@cryptopapi997

Copy link
Copy Markdown
Contributor

Picking up on #3006 since we need this for Arcium.

Adds request_threadsafe which creates a threadsafe request for usage in tokio. Fully backwards compatible.

Idk if #3053 breaks this, so would be good to get this PR merged before that one. Lmk what you think @acheroncrypto

@vercel

vercel Bot commented Jun 26, 2024

Copy link
Copy Markdown

@cryptopapi997 is attempting to deploy a commit to the coral-xyz Team on Vercel.

A member of the Team first needs to authorize it.

@acheroncrypto acheroncrypto left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks. I haven't tried it locally, but it looks great in terms of implementation other than a couple of small issues/questions I mentioned.

Fully backwards compatible.

It's mostly backwards compatible, but since RequestBuilder is public, introducing a new generic can be a breaking change for people who implement their own trait for that type (low probability).

Comment thread client/src/lib.rs Outdated
Comment thread client/src/lib.rs Outdated
Comment thread client/example/Cargo.lock Outdated
@cryptopapi997

Copy link
Copy Markdown
Contributor Author

Adapted according to your review:

It's mostly backwards compatible, but since RequestBuilder is public, introducing a new generic can be a breaking change for people who implement their own trait for that type (low probability).

True, although even in this case the fix is relatively easy. It's just a matter of implementing it for RequestBuilder<'a, C, Box<dyn Signer + 'a>> instead of RequestBuilder<'a, C>>.

Wdyt about having only request based on the async feature?
This would be more breaking imo, as anyone who uses request with async would have to switch to request_threadsafe which requires you to use an Arc<ThreadSafeSigner> instead of any signer like is currently the case.

Lmk what you think @acheroncrypto

@acheroncrypto

Copy link
Copy Markdown
Collaborator

True, although even in this case the fix is relatively easy. It's just a matter of implementing it for RequestBuilder<'a, C, Box<dyn Signer + 'a>> instead of RequestBuilder<'a, C>>.

All SemVer violations can potentially result in build errors for downstream projects, no matter how easy the fix is e.g. #3044. It's better to be safe in this case, and consider this a breaking change.

@cryptopapi997

Copy link
Copy Markdown
Contributor Author

All SemVer violations can potentially result in build errors for downstream projects, no matter how easy the fix is e.g. #3044. It's better to be safe in this case, and consider this a breaking change.

You're right. In this case, replacing request with request_threadsafe in async seems ok.

@cryptopapi997

Copy link
Copy Markdown
Contributor Author

All done, take a look @acheroncrypto

Comment thread client/src/blocking.rs Outdated
Comment thread client/src/lib.rs Outdated

@acheroncrypto acheroncrypto left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks, it looks great now! However, there is an unrelated problem: solana-labs/solana-program-library#6897 is still not resolved, so we might have to make another patch release soon depending on the resolution of that issue. In that case, it would be easier to merge this PR afterwards since this is a breaking change (and I don't want to deal with multiple branches).

We shouldn't need a patch release if they just do the solution in #3044 (comment) though.

@cryptopapi997

Copy link
Copy Markdown
Contributor Author

Since #3044 is now resolved, could you make the CI rerun so we can get this merged? @acheroncrypto

@cryptopapi997

Copy link
Copy Markdown
Contributor Author

Also this PR needs the "breaking" tag

Comment thread CHANGELOG.md Outdated
Comment thread CHANGELOG.md
cryptopapi997 and others added 2 commits July 11, 2024 19:24
Co-authored-by: acheron <98934430+acheroncrypto@users.noreply.github.com>

@acheroncrypto acheroncrypto left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks great, thanks!

@acheroncrypto acheroncrypto merged commit f677742 into otter-sec:master Jul 12, 2024
@cryptopapi997 cryptopapi997 deleted the tokio-anchor-client branch July 12, 2024 07:52
@Adrena-Corto

Copy link
Copy Markdown

Suffering on this, but I cannot use the latest anchor due to Solana version constraints. Not sure what are my options 🥴

@cryptopapi997

Copy link
Copy Markdown
Contributor Author

You basically have two options in that case:

  • Update Solana so you can use the latest version
  • Fork the anchor version you're using and apply the changes from this PR to your fork. This shouldn't be too difficult since fortunately these changes are pretty independent of Solana itself.

Downside is you then have to maintain your own fork, so whichever of these two options is the least effort

Otter-0x4ka5h pushed a commit to Otter-0x4ka5h/anchor that referenced this pull request Mar 25, 2026
…tter-sec#3057)

Co-authored-by: acheron <98934430+acheroncrypto@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants