The wasmcloud:keyvalue api has several inconsistencies and spots where it could be improved. These are a few opinionated suggestions.

1. Don't have implementation-defined results

   • ListClear : may or may not delete the list
   • Range parameters may be open or closed, implementation-defined
   • The interface should be defined with clear meanings for parameters and results. The provider implementer should implement the interface spec, even if it means they have to do extra code to adapt the interface to a particular back-end.
   • If the spec is indeterminate, then a developer either won't use it, or they have to depend on a specific implementation, negating the value of a generic interface.

2. This API is not a very generic key-value store. Many of its functions are relatively unique to Redis. A "lowest-common-denominator" type of interface would more closely model a hashmap, or a btreemap if you want sorted keys.

3. Clear() was defined to return a String. Why not nothing?

4. Get returns two values: (string value + boolean exists) - replace with "struct MaybeValue { value: Option }"

5. Set and Del return a string (which is implemented to return the request key, which the caller already has). A cleaner result would be to return nothing (void), or, alternately, implement them to return Option with the previous value, if it existed

6. List methods naming consistentency:

   • ListDel (rename from Clear and clarify that it deletes the list after removing items),
   • ListItemAdd (rename from Push),
   • ListItemDel (rename from ListItemDelete)
   • ListiItemExists (new, parallel to KeyExists)

7. Set methods naming consistency: SetDel, SetItemAdd, SetItemDel, SetItemExists, parallel to List methods

8. Range improvements:

   • rename existing Range to ListItemRange
   • add range query for values: KeyRange (return range of keys), ValueRange (return range of KV pairs). For both KeyRange and ValueRange, rage start/end should be strings.

9. All values (including list and set items) should be type blob .(non-empty array of bytes), so keyvalue can store arbitrary data, not String, which is limited to utf-8.

10. All range queries should use pagination

develop CI to automate updating smithy interfaces and documentation in the gh-pages site and

For example, here's a normal usage of the log macros

#[async_trait]
impl HttpServer for LogtestActor {
    async fn handle_request(
        &self,
        ctx: &Context,
        req: &HttpRequest,
    ) -> std::result::Result<HttpResponse, RpcError> {
        let text = form_urlencoded::parse(req.query_string.as_bytes())
            .find(|(n, _)| n == "name")
            .map(|(_, v)| v.to_string())
            .unwrap_or_else(|| "World".to_string());

        wasmcloud_interface_logging::info!("Neat! works fine"); /// <- here

        Ok(HttpResponse {
            body: format!("Hello {}", text).as_bytes().to_vec(),
            ..Default::default()
        })
    }
}

This works fine as expected, but as actors get larger it's usually desirable to split functionality off into helper functions. So when one tries to do this they get:

error[E0728]: `await` is only allowed inside `async` functions and blocks
  --> src/lib.rs:46:5
   |
45 | fn foo() {
   |    --- this is not `async`
46 |     info!("hey, why doesn't this work?");
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ only allowed inside `async` functions and blocks
   |
   = note: this error originates in the macro `info` (in Nightly builds, run with -Z macro-backtrace for more info)

Then when they try to make it async:

error[E0277]: the `?` operator can only be used in an async function that returns `Result` or `Option` (or another type that implements `FromResidual`)
  --> src/lib.rs:50:5
   |
49 |   async fn bar() {
   |  ________________-
50 | |     info!("Woah, no compile?");
   | |     ^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot use the `?` operator in an async function that returns `()`
51 | | }
   | |_- this function should return `Result` or `Option` to accept `?`
   |
   = help: the trait `FromResidual<Result<Infallible, wasmbus_rpc::error::RpcError>>` is not implemented for `()`
   = note: this error originates in the macro `info` (in Nightly builds, run with -Z macro-backtrace for more info)

And then, if they forget async and do end up returning an RpcResult

error[E0728]: `await` is only allowed inside `async` functions and blocks
  --> src/lib.rs:54:5
   |
53 | fn baz() -> RpcResult<()> {
   |    --- this is not `async`
54 |     info!("Woah, not here either?");
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ only allowed inside `async` functions and blocks
   |
   = note: this error originates in the macro `info` (in Nightly builds, run with -Z macro-backtrace for more info)

If they have their own result type that doesn't have an implementation from the error type to wasmbus_rpc::error::Error, then the error shows as well (@mattgilbride ran into this)

This is mostly to provide a few examples where developers are commonly going to hit errors trying to log outside of the actor handlers itself. The following bit of code helps, but requires constructing a Struct and passing ctx around.

        let log_sender = LoggingSender::new();
        log_sender
            .write_log(
                ctx,
                &LogEntry {
                    level: "info".to_string(),
                    text: "my text".to_string(),
                },
            )
            .await?;

Ideally, log macros should be able to be called from synchronous contexts that don't return results (a sync, void macro)

Right now the logging interface macros all use ?.await (e.g. the debug!, info!, etc). This forces these macros to only be usable inside a function that returns a Result/RpcResult. Rather than enforcing this, the macros should gracefully do nothing if logging attempts fail (because, after all, what other recourse does an actor have if logging fails? It can't log the failure...).

This lets developers use these macros in functions that don't return results.

Many of our interfaces don't have READMEs which make them difficult to approach, we should document them and provide simple examples.

the default implementation already sets status to 200 so the ok() constructor isn't needed.

This is a Rust only request

Right now the put_object and put_chunk functions use the Chunk object. This object requires the bytes to be an owned Vec<u8>. Because the data on incoming requests (like from an HttpRequest from wasmcloud:httpserver) isn't owned, this means that the data has to be cloned to send to the blobstore. So if someone sends a 20 MB file (just as an example), we then have to allocate another 20MB of memory in order to send things.

There are two possible solutions (both of which should be implemented IMO)

1. This is the easier one. Make the blobstore contract support borrowed data for Chunk. This should probably be done either way
2. This is the harder one: Most interfaces should accept owned rather than borrowed data. So for example, the httpserver contract should change from

async fn handle_request(&self, ctx: &Context, req: &HttpRequest) -> RpcResult<HttpResponse>

to

async fn handle_request(&self, ctx: &Context, req: HttpRequest) -> RpcResult<HttpResponse>

NOTE: This could be more of a https://github.com/wasmCloud/weld issue in the end as it deals with generated interface code, but I thought it would be good to start here

Is golang still supported ?

Right now the host inventory structure assumes that there's only a single version of an actor on any given host. This is no longer accurate, as different instances of the same actor on the host can have varying claims, which includes name, revision, reference URI, and more.

This needs to be updated so that only the information fixed to a given actor is in the actor's high level information and all the other metadata is moved to the instances field.

The index.html file is manually created but it has repeating blocks that should come from a template.

There is probably enough data in the Makefile to have Makefile generate the list of interfaces that need to be included in the index file. Makefile should have a rule to regenerate index.html.

As a code standard we don't want unwrap() anywhere unless there is documentation explaining why a panic isn't possible.
In this case, the return value from json should be a Result. If the caller believes that their data is guaranteed to serialize correctly, the caller can use unwrap().

Part of the value proposition of the actor model, and by extension wasmCloud, is that actors are all internally synchronous. They are single-threaded and developers don't have to worry about writing asynchronous code. Unfortunately, because Rust makes it a little too easy to compile certain things inside wasm32, it was pretty easy to make the generated code produce async stuff that can be used by either side of the provider/actor exchange.

What I'd like to see is that anything that an actor might use from the generated output be purely synchronous without any use of the async keyword. It should still be possible to allow the capability providers to utilize an async version of the trait with conditional compilation.

I suspect that in the absence of async, the process of building actors will be simpler, faster, and easier to learn for newer Rust developers. Further, I also suspect that without having to compile a single-threaded executor like tokio into the wasm module will reduce the overall actor size as well.