feat: add BorshSerializeAsync/BorshDeserializeAsync (WIP)

[DanikVitek]

The conversation started in #150.
async-generic PR: scouten/async-generic#17

This implementation should be more flexible, as it allows for different runtimes and provides traits for manual implementation in case of any uncommon runtime.

[DanikVitek]

Should the BorshDeserializeAsync and BorshSerializeAsync extend their sync counterparts and contain only async functions, or should they copy synchronous functions?

[DanikVitek]

To support deserialize_with functionality for #[derive(BorshDeserializeAsync)], I guess, there should be a separate deserialize_with_async param. Same for the serialize counterpart

[dj8yfo]

@DanikVitek is this doable without async_trait import?
Not that's its too important to get rid of it, just want to know what are limitations not allowing to get rid of it, what are potential tradeoffs, etc.

[DanikVitek]

@DanikVitek is this doable without async_trait import? Not that's its too important to get rid of it, just want to know what are limitations not allowing to get rid of it, what are potential tradeoffs, etc.

I guess, if there is no need for the traits to be dyn-compatible, then the only difficulty is making the async_generic generate async functions as sync functions, which return a custom impl Future, because the set MSRV does not yet support async functions in traits. As I can see, the BorshSerialize is not dyn-compatible, and there is no need for BorshDeserialize to be dyn-compatible, so I guess, the same could be true for the async counterparts.

If the MSRV bump is not an issue, then it's even easier

[DanikVitek]

Ok, no. Both async in traits and -> impl Trait in traits (and hence, impl Future) require the MSRV 1.75. So all return types would still need to be pin-boxed and now we're just reimplementing the async_trait at this point

[dj8yfo]

@DanikVitek can you support your assertion that the return types will need to be pin-boxed anyway by doing a minimal example in a repo outside of this pr with the right trait (BorshSerializeAsync/BorshDeserializeAsync) signatures and msrv 1.75? I think the common convention is that you cannot bump MSRV in patch releases, but in minor version releases you're free to do so.

---

EDIT: if so (if they need to be pin-boxed anyway), i think we'll stick with async_trait approach for now, as we'll mark this feature unstable__async anyway to have some freedom in modifying it in the near future and middle-term

[dj8yfo]

Should the BorshDeserializeAsync and BorshSerializeAsync extend their sync counterparts and contain only async functions, or should they copy synchronous functions?

I think the traits should be completely separate and not extend sync counterparts and lib user should be free to only implement/derive a subset or all of them if he/she desires. I was under the impression it's the case now, maybe i'm mistaken.

[dj8yfo]

To support deserialize_with functionality for #[derive(BorshDeserializeAsync)], I guess, there should be a separate deserialize_with_async param. Same for the serialize counterpart

Before diving into doing derive for new traits, i'd stop and do some tests with snapshots. @DanikVitek, can you choose some subset of types, which you consider core to borsh , and make sure the async counterparts serialize/deserialize to the same snapthos that the sync interface produces in tests? You're free to modify test code, as long you double check that existing snapshots aren't touched/renamed. Ideally this should be done by pure addition (no replacements/deletions) of test code (not modifying existing tests) and just checking the new snapshots are equal to older ones. This way it would be easy to verify during review that existing serialization format, provided by stable borsh subset, hasn't changed. If the existing test code changes, it kind of needs more attention to verify the old tests still run at all.

It might be somewhat tedious to cover all existing (snapshot) tests at once, but that's one of the reason the feature should be made unstable__ for a while to kind of stress that sync and async interfaces may produce different results due to bugs/typos.

[dj8yfo]

To support deserialize_with functionality for #[derive(BorshDeserializeAsync)], I guess, there should be a separate deserialize_with_async param. Same for the serialize counterpart

It appears that all of bounds, serialize_with, deserialize_with proc-macro sub-attributes should be cloned to _async counterparts, maybe sharing some part of implementation with their sync versions. Maybe some other sub-attributes too, but these are the ones that come off the top of my head.

[frol]

@DanikVitek Amazing job! I'd like to also invite you to join @race-of-sloths

[DanikVitek]

@race-of-sloths sounds nice

[race-of-sloths]

@DanikVitek Thank you for your contribution! Your pull request is now a part of the Race of Sloths!
New Sloth joined the Race! Welcome!

Current status: waiting for scoring

We're waiting for maintainer to score this pull request with @race-of-sloths score [0,1,2,3,5,8,13] command. Alternatively, autoscoring [1,2] will be applied for this pull request

What is the Race of Sloths

Race of Sloths is a friendly competition where you can participate in challenges and compete with other open-source contributors within your normal workflow

For contributors:

• Tag @race-of-sloths inside your pull requests
• Wait for the maintainer to review and score your pull request
• Check out your position in the Leaderboard
• Keep weekly and monthly streaks to reach higher positions
• Boast your contributions with a dynamic picture of your Profile

For maintainers:

• Score pull requests that participate in the Race of Sloths and receive a reward
• Engage contributors with fair scoring and fast responses so they keep their streaks
• Promote the Race to the point where the Race starts promoting you
• Grow the community of your contributors

Feel free to check our website for additional details!

Bot commands

• For contributors
  • Include a PR: @race-of-sloths include to enter the Race with your PR
• For maintainers:
  • Invite contributor @race-of-sloths invite to invite the contributor to participate in a race or include it, if it's already a runner.
  • Assign points: @race-of-sloths score [1/2/3/5/8/13] to award points based on your assessment.
  • Reject this PR: @race-of-sloths exclude to send this PR back to the drawing board.
  • Exclude repo: @race-of-sloths pause to stop bot activity in this repo until @race-of-sloths unpause command is called

[DanikVitek]

@DanikVitek can you support your assertion that the return types will need to be pin-boxed anyway by doing a minimal example in a repo outside of this pr with the right trait (BorshSerializeAsync/BorshDeserializeAsync) signatures and msrv 1.75? I think the common convention is that you cannot bump MSRV in patch releases, but in minor version releases you're free to do so.

@dj8yfo
I've created a separate branch with an example implementation of BorshDeserializeAsync, and it was literally a manual expansion of async_trait. Also, here's the warning I've got from the compiler. It makes sense, because async fn in traits is a syntactic sugar for fn () -> impl Future<Output=...>, so there is no way to specify additional bounds like Send or lifetimes. Therefore I think, that for performance reasons it is better to use the impl Future rather then Pin<Box<dyn Future>> as the BorshSerialize/Deserialize traits are not ment to be dyn-compatible, and implement the boilerplate code duplication via the async_generic macro

[dj8yfo]

@DanikVitek i believe the lint https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/async_fn_in_trait/static.ASYNC_FN_IN_TRAIT.html is just a reminder to do #[trait_variant::make(Send)] .
i checked

#[trait_variant::make(Send)]
pub trait Handler {
    async fn handle(&mut self, input: &Input) -> Result<String, String>;
    async fn handle2(self, input: &'static str) -> Result<String, String>;
}

struct Type {
    pub field: String,
}

impl Handler for Type {
    async fn handle(&mut self, input: &Input) -> Result<String, String> {
        let local_var = "fsdfs sdf sdf".to_owned();
        self.field.push_str(&input.field);
        self.field.push_str(local_var.as_str());

        Ok(self.field.clone())
    }

    async fn handle2(mut self, input: &'static str) -> Result<String, String> {
        let local_var = "fsdfs sdf sdf".to_owned();
        self.field.push_str(&input);
        self.field.push_str(local_var.as_str());

        Ok(self.field)
    }
}

and it appears to run. Not hundred % sure it's gonna work with async borsh traits, but looks like Box::pin in async-trait is needed to return a future from an async block + stay dyn-compatible.

`

[DanikVitek]

Box::pin in async-trait is needed to return a future from an async block + stay dyn-compatible.

Yes.

To be more precise, the async block declares an anonymous type that implements Future, and every .await call transforms into an enum variant, that holds all the variables, that are used across this .await boundary. So when we box the future, the actual type gets erased and, of course, a layer of indirection is added.

I don't completely understand the behaviour of .poll(), but it performs a non-blocking check of the future's current state

[DanikVitek]

trait_variant looks interesting. An official workaround crate

[dj8yfo]

Should rustfmt CI check use nightly? It's required for imports_granularity and group_imports

Nope, it should use stable, as it does.

I suggest that you remove nightly-only toggles from rustfmt.toml in order to not clutter the log, as it clutters a local log (besides ci) too, when running.
I usually don't commit Justfile.local to projects either, and ignore them in .git/info/exclude

[dj8yfo]

pr is in an awe-inspiring state with respect to quality. left some comments.

the only desired change from async-generic is to be able to dispatch doc-comments for sync/async variants (not really sure if it can or cannot do so already).
any other changes are not seen as required for borsh.

didn't look through all of the snapshots and some pair-wise comparisons of those (some number of combinations stays pending), but code-wise the review is mostly complete

[dj8yfo]

test: i guess such a line can be added, as it compile derives don't depend on anything in tokio and async-std

cargo test --features derive,unstable__async 'compile_derives::async_derives'

[dj8yfo]

rant: these can be deleted for the current state of tests, as there's nothing runnable, but then they'll have to be re-added (anyway) for roundtrip tests

[dj8yfo]

rant: i don't see the reason for raising minimal version, though there was no comment left in toml file before this why a specific minimum is required too )

[DanikVitek]

This syn version has MSRV of 1.61, so it should be compatible. A couple of new patches has come out, so I could set it higher

[DanikVitek]

@dj8yfo async-generic does support dispatching attributes on sync and async items separately, including doc comments. All features of the async-generic will be documented.
Could you, please, mention all the syntax propositions in this PR scouten/async-generic#17, for them to not get lost among the review comments?

[DanikVitek]

@dj8yfo Is this an OK update?