Skip to content
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

feat: Add tokio support to anchor_client RequestBuilder #3057

Merged
merged 12 commits into from
Jul 12, 2024

Conversation

cryptopapi997
Copy link
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

Copy link

vercel bot commented Jun 26, 2024

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

A member of the Team first needs to authorize it.

Copy link
Collaborator

@acheroncrypto acheroncrypto left a comment

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).

client/src/lib.rs Outdated Show resolved Hide resolved
client/src/lib.rs Outdated Show resolved Hide resolved
client/example/Cargo.lock Outdated Show resolved Hide resolved
@cryptopapi997
Copy link
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
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
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
Contributor Author

All done, take a look @acheroncrypto

client/src/blocking.rs Outdated Show resolved Hide resolved
client/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@acheroncrypto acheroncrypto left a comment

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
Contributor Author

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

@cryptopapi997
Copy link
Contributor Author

Also this PR needs the "breaking" tag

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@acheroncrypto acheroncrypto left a comment

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 coral-xyz:master Jul 12, 2024
48 of 49 checks passed
@cryptopapi997 cryptopapi997 deleted the tokio-anchor-client branch July 12, 2024 07:52
@Adrena-Corto
Copy link

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

@cryptopapi997
Copy link
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

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