Simplifying client API

[loziniak]

Hello. I’m writing a Safenet FFI layer for integration with Red language. It includes a very simple Rust parser to extract structure of modules, objects, methods and parameter types. It’s then used to generate FFI code in Rust and Red. Communication between Rust and Red is done by serializing to redbin format

Unfortunately, Safenet client APIs are getting more and more complicated lately. Is there a possibility to refactor it a little to keep it simple? Main things, that give me a headache are:

1. Complex parameter types with generics like in WalletClient::pay_for_records or WalletClient::pay_for_storage.
2. Having params of types that don’t implement de-/serialization and/or Debug trait, like Client in create* methods of ClientRegister (Perhaps they could just be moved to Client struct?) or WalletClient parameters of Files sruct methods (Files already contains Client reference, so no need to give entire WalletClient, perhaps just LocalWallet would do?). But perhaps I’d just have to implement transferring some parameters as references instead of serializing anyway.
3. Structure of modules and exports. Perhaps a number of combinations of pub, use and mod could be limited? And maybe also getting rid of super and crate would be possible?

My generator works on templates, so if we could get it right, I think generating bindings for more languages could be possible. And also as additional benefit, simple APIs would allow less experienced Rust developers to build for Safe.

I’d happily discuss it in more details here, or perhaps GitHub issues would be a better place for further arrangements?

[joshuef]

@loziniak

Complex parameter types with generics like in WalletClient::pay_for_records or WalletClient::pay_for_storage.

This will likely get refactored away down the line. (it should be possible to store that data->payment map in the wallet itself)

@loziniak

Having params of types that don’t implement de-/serialization and/or Debug trait, like Client in create* methods of ClientRegister

If it’s simple to add these derived traits, a PR here would be welcome!

@loziniak

Perhaps a number of combinations of pub, use and mod could be limited? And maybe also getting rid of super and crate would be possible?

super and crate are very useful for referencing things in the crate we are in. It’s pretty standard rust. What’s the issue with those? (and why might you need a limit? I don’t fully understand here)

[loziniak]

@joshuef

This will likely get refactored away down the line

@joshuef

If it’s simple to add these derived traits, a PR here would be welcome!

I’ll try my best! Probably instead of serializing Client I’ll submit a PR with passing LocalWallet as parameter and transfer Client as a reference.

@joshuef

super and crate are very useful for referencing things in the crate we are in. It’s pretty standard rust. What’s the issue with those? (and why might you need a limit? I don’t fully understand here)

Now I actually don’t exactly parse full module structure and get by with it Some simplifications in API would probably allow me to keep it as it is. But if it’s additional work, then probably not worth it – if it’s either more work for me vs. you, then I’ll probably go with full module structure myself. That would mean not distracting the Team from the launch, which is the main focus for us all.

Thanks!

[joshuef]

@loziniak

Probably instead of serializing Client I’ll submit a PR with passing LocalWallet as parameter and transfer Client as a reference.

I’ve actually gotten rid of local wallet in some client apis now, so maybe hold off there!

[loziniak]

I get errors like this on output/sn_ffi/src/lib.rs for every type from sn_dbc crate (Dbc, DbcId, Token, PublicAddress):

error[E0308]: mismatched types
   --> src/lib.rs:173:60
    |
173 |         rt.block_on(Client::get_spend_from_network(client, &params.0));
    |                     ------------------------------         ^^^^^^^^^ expected `sn_dbc::dbc_id::DbcId`, found `DbcId`
    |                     |
    |                     arguments to this function are incorrect
    |
    = note: `DbcId` and `sn_dbc::dbc_id::DbcId` have similar names, but are actually distinct types
note: `DbcId` is defined in crate `sn_dbc`
   --> /home/maciek/.cargo/registry/src/github.com-1ecc6299db9ec823/sn_dbc-19.1.1/src/dbc_id.rs:24:1
    |
24  | pub struct DbcId(PublicKey);
    | ^^^^^^^^^^^^^^^^
note: `sn_dbc::dbc_id::DbcId` is defined in crate `sn_dbc`
   --> /home/maciek/.cargo/registry/src/github.com-1ecc6299db9ec823/sn_dbc-20.0.0/src/dbc_id.rs:24:1
    |
24  | pub struct DbcId(PublicKey);
    | ^^^^^^^^^^^^^^^^
    = note: perhaps two different versions of crate `sn_dbc` are being used?
note: method defined here
   --> /mnt/share/prj/maidsafe/github/safe_network/sn_client/src/api.rs:353:18
    |
353 |     pub async fn get_spend_from_network(&self, dbc_id: &DbcId) -> Result<SignedSpend> {
    |                  ^^^^^^^^^^^^^^^^^^^^^^

I think I’m importing right structures, but compiler still complains…

[loziniak]

What can be a reason for that?

[loziniak]

@joshuef

@loziniak

types that don’t implement de-/serialization

simple to add these derived traits

I have similar problem with tokio::sync::OwnedSemaphorePermit. Is it there to stay, or it will also be refactored out of API?

[loziniak]

Another request:
Could it be made that client::send() and client::send_without_verify() returned sn_client::Error instead of sn_transfers::wallet::Error? I did not come across other methods, that behave like this.

(edit) It’s not needed anymore, since I convert error enums to strings anyway for serialization.

[joshuef]

@loziniak

I have similar problem with tokio::sync::OwnedSemaphorePermit. Is it there to stay, or it will also be refactored out of API?

This has largely gone now I think

@loziniak

Could it be made that client::send() and client::send_without_verify() returned sn_client::Error instead of sn_transfers::wallet::Error? I did not come across other methods, that behave like this.

That seems reasonable that they should wrap that err at that level, yeh