Skip to content

Make PreparedSend cloneable #687

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ok300
Copy link
Contributor

@ok300 ok300 commented Mar 26, 2025

Description

This PR makes PreparedSend cloneable. This is useful for callers when handling the send cancellation, which may happen in a context that only has a &PreparedSend.


Checklist

@ok300 ok300 changed the title Make PreparedSend cloneable Make PreparedSend cloneable Mar 26, 2025
@thesimplekid
Copy link
Collaborator

#596 (comment)

@ok300
Copy link
Contributor Author

ok300 commented Mar 26, 2025

I don't know if this is a good defense against the flow you mentioned.

For example, when this will be exposed via bindings, there is no way to enforce the same rust-specific ownership protections in let's say javascript. So it's safe to assume at least some callers will be able to clone with no problem.

IMO the more robust way is to make this cloneable (since we cannot reliably forbid it) and rely on a different mechanism to avoid prepare_send -> send -> cancel_send. The comment you linked says such mechanisms were already added to that PR.

@davidcaseria
Copy link
Contributor

What is the use case for which Clone is needed?

Because JavaScript users can do something dangerous, we should allow Rust users to do something dangerous isn't a compelling reason to allow this, imo.

@ok300
Copy link
Contributor Author

ok300 commented Mar 27, 2025

Most apps are multi-threaded (UI thread, background threads), so they pass data either by reference with Arc or by cloning. The place calling prepare_send might well be a different thread or context than the one calling send, since prepare_send might involve asking for user confirmation. IIRC this was the main argument for the current Prepare Send architecture, namely that in some apps different threads could try to send or swap at the same time, like bots or automated tools.

So it's therefore natural that callers will try to either

  1. use PreparedSend as a reference, which won't work because send and cancel_send take ownership, or
  2. clone PreparedSend when passing it to the thread or context that consumes it, which won't work because its not cloneable.

If that's not possible, they'll have to

  1. rework their app internals to allow passing ownership of PreparedSend (for rust, that's channels or an Arc<Mutex<>>)

In rust, callers can be blocked from the first 2 options and essentially forced to use the 3rd.

But CDK is planning to add bindings. Do you know for sure which languages will be covered by the bindings? Will those users be doing something "dangerous" if they try to clone an object or use it as reference, which they already do in the rest of their app? Is the CDK team prepared to 1) document why this shouldn't be attempted, 2) keep an eye out for all future binding languages and make sure the exposed PreparedSend is not cloneable in that language?

Seems to me a bit unfair to callers, and not a very robust way forward for CDK.

Since we can't reliably forbid it, we should allow cloning and instead rely on CDK-internal mechanisms to handle the prepare / send / cancel_send flow.

@davidcaseria
Copy link
Contributor

I haven't encountered this issue when building CDK-Flutter using a widget or the Wallet Rust struct and Dart class.

If someone needed to use a prepared send across multiple threads, although I can't think of a reason why anyone would do that, using a Mutex seems reasonable to me to avoid trying to process a send twice.

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

Successfully merging this pull request may close these issues.

3 participants