Traits or no traits

[matklad]

There are two alternative ways we can design the overall shape API.

One is to make the Worker type to be parametrized by network type. So things like Sandbox and TestNet are different types. The public API would look roughly like this:

 /// Worker holds an actual network, a distinct type
 pub struct Worker<T> { }

/// Methods which can be specific to some network are abstracted away in a
 /// trait.
 #<span class="error">[async_trait]</span>
 pub trait TopLevelAccountCreator 

 { async fn create_tla(&self, id: AccountId, signer: InMemorySigner) -> Result<CallExecution<Account>>; async fn create_tla_and_deploy(&self, id: AccountId, signer: InMemorySigner, wasm: Vec<u8>) -> Result<CallExecution<Contract>>; } 

/// Worker has extra methods via trait if the underlying network supports it,
 /// checked at compile time
 #<span class="error">[async_trait]</span>
 impl<T> TopLevelAccountCreator for Worker<T>
 where
 T: TopLevelAccountCreator + Send + Sync,

 { // Delegates } 

/// Methods available on all of the networks are inherent.
 impl<T> Worker<T>
 where
 T: NetworkClient,
 {
 pub async fn call(&self, contract: &Contract, method: String, args: Vec<u8>, deposit: Option<Balance>) -> Result<CallExecutionDetails> { }
 pub async fn view(&self, contract_id: AccountId, method_name: String, args: FunctionArgs) -> Result<serde_json::Value> { }
 }

pub struct Sandbox { }
 impl NetworkClient for Sandbox {}
 #<span class="error">[async_trait]</span>
 impl TopLevelAccountCreator for Sandbox {}

pub struct Testnet { }
 impl NetworkClient for Sandbox {}
 // NB: this impl is **not** provided.
 // #<span class="error">[async_trait]</span>
 // impl TopLevelAccountCreator for Sandbox {}

The usage would look like this:

 use workspaces::

 {Worker, Sandbox, TopLevelAccountCreator} 

;

fn main() 

 { let w: Worker<Sandbox> = Worker::new(); .... } 

fn generic_usage<T: TopLevelAccountCreator + NetworkClient>(w: Worker<T>) 

 { ... } 

An alternative is to not make type-level distinction between the types of network. The public API would then look like so:

 /// Worker holds a JSON rpc clinet, the same type for all networks.
 pub struct Worker{ }

impl Worker {
 /// What network the worker is for is determined at runtime, rather than by
 /// a type parameter.
 pub fn new_testnet() -> Worker {}
 pub fn new_sandbox() -> Worker {}

 /// It's possible to call any method on any worker, but that will result in
 /// an "unsupported method" error at runtime, if the underlying network
 /// doesn't support the method
 pub async fn create_tla(&self, id: AccountId, signer: InMemorySigner) -> Result<CallExecution<Account>>;
 pub async fn create_tla_and_deploy(&self, id: AccountId, signer: InMemorySigner, wasm: Vec<u8>) -> Result<CallExecution<Contract>>;

 pub async fn call(&self, contract: &Contract, method: String, args: Vec<u8>, deposit: Option<Balance>) -> Result<CallExecutionDetails> { }
 pub async fn view(&self, contract_id: AccountId, method_name: String, args: FunctionArgs) -> Result<serde_json::Value> { }
 }

The usage would look like this:

 use workspaces::Worker;
 fn main() 

 { let w = Worker::new_sandbox(); } 

fn generic_usage(w: Worker) {

}

fn runtime_generic_usage {
 let w = if std::env::args().contains("--testnet") 

 { Worker::new_testnet() } 

 else 

 { Worker::new_sandbox() } 

;
 }

The question is, which one should we choose.

Traits:

Pros:

• compiler checks that operations "make sense"
• it's possible (though not super trivial) so see which specific operations differentiate networks

Cons:

• Significatly larger public API: there are many Rust names the user is exposed to
• Requires many imports for the user to use
• Somewhat high-brow language maichnery – the API is generic and has trait bounds, rather than being just inherent methods which are always available
• Adds public dependency on asyc_trait, which is cool, but is fundamentally a hack.
• Just overall is significantly more wordy
• It might be hard to get traits-based API right on the first try, as traits have more langauge rules around them (restrictions regarding dynamic dyspatch, async in traits, coherence, etc).

No Traits:

Pros:

• as simple an API as one can get – a single type with a bunch of methods
• no need to import lots of stuff – you can strart with Worked and autocomplete anything to victory
• if users wants to write the code which can work with several networks, users don't have to make that code generic and figure out whchi trait bounds they need
• users can make descision about the network at runtime (see runtime_generic_usage)

Cons:

• user might do something wrong like Worker::new_mainnet().patch_state() and get an error at runtime, rather than at compile time.

[willemneal]

Having your patch state error at runtime is not so bad. Ideally for these cases you would create an interface that forced a sandbox environment, for example a test function that is passed a sandbox worker.

I agree with @matklad that the simpler we can keep the API the better. Others can make traits on top of this if they so desire.

[ChaoticTempest]

Having your patch state error at runtime is not so bad. Ideally for these cases you would create an interface that forced a sandbox environment, for example a test function that is passed a sandbox worker.

Yeah, so that's what I have going with right now with these traits. If we really want to eliminate as many traits as we can, we can get rid of StatePatcher here and explicitly say that if we want a network that patches state, then it has to be a Worker<Sandbox> type.

fn generic_usage<T: TopLevelAccountCreator + NetworkClient>(w: Worker)

Well to be fair, the usage isn't that fine grained: we'd just be looking at generic_usage<T: Network>(...). But in any case, I can minimize TopLevelAccountCreator, NetworkClient and NetworkInfo all into Network. Originally were all separate just so we can see where the boundaries were if it didn't make sense for a Network to have top level account creation, but all networks need it either ways. So we can minimize all these into one Network trait.

There's one more requirement I think needs to fit the bill here too, but might lean into over engineering: we need to allow custom networks later (like when RPC as a service becomes a thing and would require different RPC endpoints or if a user wants their own separate RPC service). That's where I think these traits come in handy, since we can now generalize over a set of types/networks instead of requiring us to hardcode the concrete type itself.

Alternatively, we could have a trait object underneath but we then lose specific network info (unless we want to introduce downcasting to the mix...). Overall, I think that requiring a type parameter for Worker is necessary at the very least. I'm down for simplification, but I think we'd end up back here again at square one in the future.

Also, one thing to note is that there is differing behavior between how sandbox vs testnet/mainnet does top level account creation, so we do need an interface around that. I'm sad we can't go about it in a more config/data-driven way, but that's also one of the reasons why I went with traits originally.

[matklad]

Yeah, so that's what I have going with right now with these traits. If we really want to eliminate as many traits as we can,

Not exactly: the primary angle I am looking from is "are we forcing the user's code to be generic at compile time, or polymorphic at runtime"? The number of traits doesn't matter pre se, it's the fact whether worker has a type parameter or not, and, consequently, whether if runtime_condition { Worker::new_testnet() } else { Worker::new_sandbox() } pattern is possible.

we need to allow custom networks later

Hm, but why we need custom types for each network, rather than custom runtime config? Why something like Worker::new_custom_rpc("https://rpc.as.a.service:1234") wouldn't be enough? We definitely need some flexibility here, but we don't necessary need to provide flexibility via compile time toggles. Compile-time flexibility generally comes with a cost of more complicated API.

Also, one thing to note is that there is differing behavior between how sandbox vs testnet/mainnet does top level account creation,

Yeah, we definitely need to implement different behavior here, but it seems like an implementation concern here, rather than than API concern. Ie, if we just want to have different impl for test net and sandbox, but do want to use traits internally, we can do something like the following:

// NB: the only pub thing
pub struct Worker { repr: WorkerRepr }

enum WorkerRepr {
    Sandbox(Sandbox),
    Testnet(TestNet)
}

trait TopLevelAccountCreator {}
impl TopLevelAccountCreator for Testnet {} 
impl TopLevelAccountCreator for Sandbox {} 

[ChaoticTempest]

The number of traits doesn't matter pre se, it's the fact whether worker has a type parameter or not, and, consequently, whether if runtime_condition { Worker::new_testnet() } else { Worker::new_sandbox() } pattern is possible.

ahh got it. Thought you wanted to eliminate the amount of traits since they were excessive in number at first but maybe I'm mixing up comments. For the runtime, we can do the following in its current state, but requires some know-how about rust and dispatching:

let worker: Worker<Box<dyn Network>> = if condition { 
   workspaces::sandbox().into()  // or some other more intuitive conversion function name.
} else {
   workspace::testnet().into()
};

Why something like Worker::new_custom_rpc("https://rpc.as.a.service:1234") wouldn't be enough?

Yeah, this was the part I was saying about maybe leaning into over-engineering. I can see we give the ability to users to do their own custom network with custom behavior like TopLevelAccountCreator. The impl for it might not be the same as either the ones we've done for testnet or sandbox. The top level account creation for testnet/mainnet is being done through a helper contract, so the user could create their own helper contract and impl this trait for their custom network, or it could be some other process altogether. So I think having purely some configs for this wouldn't help this case.

if we just want to have different impl for test net and sandbox, but do want to use traits internally, we can do something like the following

huh I see, that might just work I think. Additionally, we can still have network specific behavior by casting/matching it to the variant:

if let Some(worker) = worker.as_sandbox() {  // might require returning a Worker<T> here though
    worker.patch_state( ... )
}

I wonder how the custom network I mentioned before would fit into the enum. Maybe something like:

enum WorkerRepr {
   ...
   Custom(Box<dyn Network>),
}
impl Worker {
   fn new_custom( ... ) {}
}

[matklad]

I wonder how the custom network I mentioned before would fit into the enum. Maybe something like:

Yeah, that's what I'd go for for an extensible interface. This is a blend of two patterns:

• Non Virtual Interface -- a pattern from C++ where you don't expose virtual function in class public API, but override them in derived classes
• More general "abstractions has two sides" pattern which I don't know a proper name of.

In a nutshell, most abstractions have two sides -- user interface intended for the consumers that use abstraction, and customization interface intended to provide several implementations of the abstraction. The two needs are often pretty orthogonal, but sometimes the same language mechanism is used to implement them both. The (anti-)pattern here is to accidentally mix the two. The pattern is to make the two separate. You last suggestion is exactly this.

/// This is fully concrete type for users of the worker. 
pub struct Worker {
    repr: Box<dyn WorkerTweaks>
}

/// This is a customization point
pub trait WorkerTweaks { ... }

impl Worker {
    /// This is the magic: we take user-supplied customizations, and wrap them in concrete types. 
    pub fn with_tweaks<T: WorkerTweaks>(t: T) { Worker { repd: Box::new(t) } }
}

But all this is basically generic philosophy :)

To get more concrete, we can make the interface to the library generic or concrete, with the primary tradeoffs being type-safety vs ease of use. I feel moderately strongly that concrete would be a better interface here, but I don't have enough context to make a judgement call here.

[ChaoticTempest]

Non Virtual Interface ...

mmm, poor man's abstract classes here we go... I'm only joking about this one

To get more concrete, we can make the interface to the library generic or concrete, with the primary tradeoffs being type-safety vs ease of use. I feel moderately strongly that concrete would be a better interface here, but I don't have enough context to make a judgement call here.

So what you're saying is that you prefer the enum variants + trait object vs purely just trait object right? I think as long as we don't expose those parts, and just have a good enough casting/matching interface or some runtime checks/guards, we can change it up later if it needs to be. I can just start out with the former and see how it goes

[matklad]

So what you're saying is that you prefer the enum variants + trait object vs purely just trait object right?

Not exactly: I don't care at all what we use internally. Maybe variants are better, maybe the trait object, but this doesn't really matter, as we can change from one to another and back at a whim, as that doesn't affect public API at all.

What I feel strongly about is just the public interface we expose: is it pub struct Worker { ... } or pub struct Worker<T: Network> { ... }, as that we won't be able to change easily (though technically we can go from Worker to Worker<T: Network> by defining struct Worker<T: Network = Sandbox>).

[ChaoticTempest]

What I feel strongly about is just the public interface we expose: is it pub struct Worker { ... } or pub struct Worker<T: Network> { ... }, as that we won't be able to change easily

Makes sense. Let's try with purely Worker without any type arguments for now then. It's going to be hard to go from one way to the other on either case, and it makes sense to hide the network details. At least there's some ways to go about it from a customization/extendibility standpoint which was the main thing for me that was dragging the public interface.

though technically we can go from Worker to Worker<T: Network> by defining struct Worker<T: Network = Sandbox>

What would've been interesting is if we could've done:

impl Network for Box<dyn Network> {}
struct Worker<T: Network = Box<dyn Network>>

but seems like it doesn't fully work for nested types or whatever it's called:

fn do_something(worker: Worker) { ... }

fn main() {
   let worker: Worker<Box<Sandbox>> = Worker::sandbox();
   do_something(worker);   // compile error
}

even though it compiles for this case:

fn do_something(workspace: Box<dyn Network) { ... }

fn main() {
    do_something(Box::new(Sandbox));
}

I think it just requires some further casting but might be pretty hacky underneath. Plus, we'd explicitly write Worker<Box<Sandbox>> in the function signature which is not so nice. Playground if you want to play with it.

[matklad]

What would've been interesting is if we could've done:

That's plausible, and that's what aws lambda does:

https://docs.rs/aws-sdk-lambda/latest/aws_sdk_lambda/client/struct.Client.html

You can think of Client as a concrete type, as it has C = DynConnector. But, if you need extra perf and what to avoid dynamic dispatch, you can do that as well. The motivation for that design there is performance as far as I can tell. For us, I don't think we should need to worry about extra runtime overhead.