invoke: add --event and --event-file flags #137

2opremio · 2022-09-16T18:23:05Z

What

Add new event-related flags to the invoke subcommand:

--events: enable printing host/debug events
--events-file: write events to a file instead of stdout

Why

Close CLI: Print events during invoke #128
Enable saving events to a file/pipe which in turn will be used by soroban-cli serve to subscribe to local events (a new, matching --events-file flag will be added to to sorobab-cli serve for that purpose). This is part of CLI: CLI Event Streaming Tool for local sandbox #39

Known limitations

N/A

Add new event-related flags to the invoke subcommand: * --events: enable printing host/debug events * --events-file: write events to a file instead of stdout

leighmcculloch · 2022-09-16T19:27:13Z

2. Enable saving events to a file/pipe which in turn will be used by soroban-cli serve to subscribe to local events (a new matching --events-file flag will be added to to sorobab-cli for that purpose)

The dependency of serve on invoke via a file/pipe seems unnecessary, and unintuitive, and breaks the "these are separate runtimes" model.

I don't think someone running soroban-cli serve should see events from soroban-cli invoke commands. Invoke and serve should be independent of each other, other than sharing the same ledger.json file for data storage if they so choose.

If a transaction is submitted to soroban-cli serve via a HTTP call then it's events should be accessible via the HTTP interface, because the events were generated within that runtime.

But if a contract is invoked via soroban-cli invoke the only place those events should go are to the command line (or to file) for the use of the invoker, not for the use of the HTTP service, because they are both different runtimes and events are ephemeral.

cc @paulbellamy Wdyt?

leighmcculloch · 2022-09-16T19:32:33Z

src/invoke.rs

+                let event_str = match event {
+                    HostEvent::Contract(e) => serde_json::to_string(&e).unwrap(),
+                    HostEvent::Debug(e) => format!("{}", e),
+                };
+                writeln!(out, "{}", event_str).map_err(|e| Error::CannotWriteEvent { error: e })?;


This looks like it will produce a file that contains invalid JSON, because debug events are not being outputted as JSON, so I don't think this file will be very usable.

It's quite unclear what the use of writing the events to file from the invoke command, which is a command intended just for a human to use. I think we could leave it out of the CLI.

I wrote the intention in the PR's description.

I should transform the debug events to JSON though

tomerweller · 2022-09-17T01:36:15Z

@2opremio if a developer doesn't supply an --events-file then events won't get plumbed to the serve http server?

leighmcculloch · 2022-09-17T02:18:56Z

@paulbellamy @2opremio The intent with the invoke command was that it could also invoke contracts on remote hosts by building a tx and submitting it to it. Originally that remote host was just going to be a Horizon. But if you'd like the invoke command to invoke a transaction in an RPC server I think that makes sense too. That's how I think it would make the most sense to integrate invoke with a running RPC server (or the serve command which is just an RPC server emulated). In this situation the runtime would be inside the serve, so the events would be generated inside the serve.

Does that achieve what you're wanting to do?

paulbellamy · 2022-09-17T08:52:16Z

breaks the "these are separate runtimes" model.

I think that's the fundamental disagreement here. I think they should be the same "runtime". If you have an rpc server running against a network, and submit a new txn there via "invoke", then you'd expect that to be interoperable. I don't see why the sandbox should be different. ledger.json is equivalent to a "network".

Wdyt?

tl;dr is I super disagree with all of that.

Edit: Let me elaborate...

The way I think of the soroban dev tooling is MVC-inspired, something like this, where grey arrows indicate a user's choice of setup:

(@leighmcculloch, I think we did a diagram similar to this at one point, but can't find it now)

From that perspective, sandbox should behave similar to any other stateful network. If you use cli invoke to call a method on futurenet, then you query that from soroban-rpc you'd see the data has changed in both places.

For sandbox, this is really important to help with locally testing your dapp. Imagine you're writing a crowdfunding campaign dapp. The workflow I'm using to test soroban-example-dapp from the user-facing side is:

# Build/deploy/initialize the contract.
# IRL, this is done via: https://github.com/stellar/soroban-example-dapp/blob/main/initialize.sh,
# but I've expanded it here to show what is going on.

# Build the crowdfund contract
$ cargo build --release --target wasm32-unknown-unknown

# Deploy the crowdfund contract
$ soroban-cli deploy --id 0 \
  --wasm target/wasm32-unknown-unknown/release/soroban_crowdfund_contract.wasm

# Initialize the crowdfund contract
$ deadline="$(($(date +"%s") + 86400))"
$ soroban-cli invoke --id 0 \
  --fn initialize \
  --arg-xdr "$admin" \
  --arg "$deadline" \
  --arg "1000000000" \
  --arg '[0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0]'

# Launch the cli serve so I have a jsonrpc server backed
# by the sandbox (for the web UI to talk to)
$ soroban-cli serve

# Launch the web app UI http server (in a separate terminal)
$ yarn dev

# Poke around in the web UI, and do a donation. A this point I
# have a working crowdfund contract in the sandbox, because I've
# called the "initialize" method on it. `serve` and the previous
# `deploy` and `invoke` all share the same context via `ledger.json`.
#
# In testing, I could:
# - Make a donation via the web UI, then check the
#   new balance via `cli invoke`
# or
# - Conclude the crowdfund via `cli invoke` then observe
#   the new status via the web UI, to check the frontend handling.

Whether (internally) that is implemented via having the cli subcommands do a pseudo-jsonrpc call to serve or not, I'm fine with whatever is easier/cleaner to implement. They should certainly share implementation code, but I'm not sure doing an internal jsonrpc call is the cleanest way to do that. Currently the code structure is not super-well set up to share implementation between cli subcommands, and jsonrpc requests. For sure that needs improvement. But the important thing for the developer workflow here is that the context between them is shared so that they can interoperate. The key concept is that the "sandbox" is just another lighter-weight "network", and should behave similarly to how a real network would.

Side-note: This mental-model is why I feel that eventually having soroban-cli serve support talking to real networks adds an element of completeness, and makes the system easier to understand.

paulbellamy · 2022-09-17T08:58:07Z

src/invoke.rs

@@ -42,6 +47,12 @@ pub struct Cmd {
    /// File to persist ledger state
    #[clap(long, parse(from_os_str), default_value(".soroban/ledger.json"))]
    ledger_file: std::path::PathBuf,
+    /// Output file to write events
+    #[clap(long, parse(from_os_str))]


Suggested change

#[clap(long, parse(from_os_str))]

#[clap(long, parse(from_os_str), default_value(".soroban/events.json"))]

wdyt?

leighmcculloch · 2022-09-17T17:46:26Z

Got it, interesting.

I think a developer will see the events file like a log file. For example, debug events (called logs in the SDK) are logged as a non-roundtrippable string because the intent is for the developer to write this code:

log!("uh oh: {}, value: {}", symbol!("hi"), 2);

and to see a human readable formatted string, like this:

uh oh: Symbol(hi), value: I32(2)

To make the output roundtrippable we'd have to output the debug event as a JSON object which would make logs not human readable as intended.

I think this means at the very least we need to separate debug events from contract and system events when configuring output destinations, so that we can continue to output human readable logs without needing them to be roundtrippable.

The second thing is that when I look at the CLI interface and see an events file it isn't intuitive to me that it is a communication mechanism. Even the ledger.json file is dubious to describe as a communication mechanism. From the perspective of the invoke I would describe it as just an input and output.

It's also worth noting that having two applications read and write from the same file seems like it will have its challenges with corruption, etc. Especially when people on a variety of platforms are using this tool (Windows, Mac, Linux, etc).

I think it is very reasonable to expect that the invoke command can affect the serve runtime and to generate events there, but I think the way it should do that is by submitting a transaction to the serve http endpoint, and then streaming back the results and events from the RPC server in the same way it will do so when invoking a contract with a real remote RPC server.

i.e. The integration between cli commands and the serve command should match the integration between cli commands and a real RPC server. The serve is after all just a local emulated version of the RPC server.

We could maybe even make this seamless to the developer by writing the serve URL to a file in .soroban/ when starting serve and deleting it when stopping serve, and having invoke default to a http request to serve when it sees that file there.

paulbellamy · 2022-09-17T18:05:44Z

@leighmcculloch I think we broadly agree, just talking past each other here.

re: having soroban-cli invoke do a jsonrpc request to serve if it is running. Seems... complicated to get reliable, and like it could break in strange ways. Doing an internal jsonrpc request seems like a hacky way to re-use code. But, if we need to in order to prevent data corruption, then fine. Maybe a better solution would be to share the implementation code, and have both processes use something like sqlite that would ensure consistency? Another dependency, though, which sucks.

As it is now, I don't expect things in the .soroban to be user-friendly/readable, so I'm not worried about the format of things there (though we could change that in future if we want). fwiw, when I initially looked at this, I had thought of using a plain text file called something like .soroban/events.log, as it is sort of a logfile. But @2opremio suggested a pipe would avoid file consistency issues, at the cost of historical and user readability. Seemed like a nice tradeoff for now.

leighmcculloch · 2022-09-17T18:19:24Z

having soroban-cli invoke do a jsonrpc request to serve if it is running. Seems... complicated to get reliable

This is the way that the dapp will communicate with the RPC server and the serve emulator, so we need a reliable http server in any case?

leighmcculloch · 2022-09-17T18:37:44Z

a pipe would avoid file consistency issues, at the cost of historical and user readability

Regarding pipes in general: Keep in mind that soroban-cli will be used on five targets, three operating systems across two architectures and pipes come with their own set of challenges such a deadlock, and may not behave the same on all operating systems.

Would the pipe be where the soroban-cli sends the JSON-RPC request? That sounds fine if you prefer that, although I think http will present less issues, and I think this creates additional work unnecessarily.

If the pipe will be a side channel for events only, this seems like a not ideal architecture, and again creates additional work unnecessarily.

The serve already exposes a transaction endpoint, and we're already adding logic to invoke to send a transaction to FutureNet in #42, so in the minimal case where a developer points invoke at their local serve it's zero additional work on top of other work we already have planned.

2opremio · 2022-09-19T13:20:42Z

If the pipe will be a side channel for events only, this seems like a not ideal architecture, and again creates additional work unnecessarily.

That was the idea, using the pipe as an one-way communication channel for events from soroban-cli invoke to soroban-cli serve

If you feel strongly about using an http endpoint instead, that's fine, but please let's make a choice and move forward. We have already spent too much time on this.

@leighmcculloch as I understand it, what you are suggesting is something like:

The user starts soroban-cli serve (listening on, say localhost:8000) , which incorporates a new endpoint (say eventsSend) which can be used to send events.
The user subscribes to events by calling the eventsSubscribe endpoint on the running soroban-cli serve from (1)
The user runs soroban-cli invoke --events-server localhost:8000 .... This results in soroban-cli invoke sending events to soroban-cli serve using the new eventsSend endpoint.
The user recieves event notifications from soroban-cli serve.

Please confirm. If that's not what you mean, please provide an explicit design and user-flow example.

Having to deal with --events-server reduces usability (the pipe location can be implicit) and adding extra http machinery for this seems like a waste of time. However, we have spent way more time discussing this, so I am willing to cave in.

leighmcculloch · 2022-09-19T14:36:37Z

@2opremio That's not what I'm suggesting.

I'm suggesting we build no side-channel for events. I'm suggesting we build what we plan to build already for invoking contracts on remote RPC servers, which serve is a local emulator for, i.e. #42.

By invoking a contract on the remote RPC server (i.e. soroban-cli serve), events should naturally be generated within the soroban-cli serve runtime, and the invoke command should be able to subscribe to events like it would any remote RPC server or dapp.

To break it down:

Use runs:

soroban-cli invoke --rpc-url http://localhost:8000 --account <G...> --id <CONTRACTID> --fn <FN> --arg ...

Contract is invoked within soroban-cli serve, in the same way the dapp will invoke a contract within serve.
soroban-cli serve will produce events in the same way it will when the dapp invokes contracts on serve.
soroban-cli invoke collects events from serve like a dapp would.

i.e. I'm suggesting we build nothing unique / bespoke for sandbox invoke with sandbox serve, and instead rely on features we already have to build for the dapp to serve integration.

2opremio · 2022-09-19T17:08:58Z

I don’t fully understand what needs to be done from your description. Do you mean that #39 shouldn’t be implemented at all?

Please be clear on the mechanics of —rpc-url. Does this mean serve will also implement the sandbox emulation? What methods are invoked?

leighmcculloch · 2022-09-19T17:17:40Z

That's what I'm saying, we don't need to build anything bespoke for the sandbox version of invoke for use with serve. The remote URL version of invoke (#42) can be used to invoke contracts on serve like it will be able to invoke contracts on any rpc server.

#39 is a completely different issue afaik, as it will stream events from a remote rpc endpoint.

leighmcculloch · 2022-09-19T17:18:38Z

Does this mean serve will also implement the sandbox emulation?

serve is an implementation of the soroban-rpc. It exposes the same API and is backed by the ledger.json. All the commands when backed by ledger.json is what we have been calling the sandbox.

invoke: add --event and --event-file flags

4d63be3

Add new event-related flags to the invoke subcommand: * --events: enable printing host/debug events * --events-file: write events to a file instead of stdout

2opremio requested a review from paulbellamy September 16, 2022 18:23

Appease clippy

3dca424

leighmcculloch reviewed Sep 16, 2022

View reviewed changes

paulbellamy reviewed Sep 17, 2022

View reviewed changes

2opremio closed this Sep 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

invoke: add --event and --event-file flags #137

invoke: add --event and --event-file flags #137

2opremio commented Sep 16, 2022 •

edited

leighmcculloch commented Sep 16, 2022 •

edited

leighmcculloch Sep 16, 2022 •

edited

2opremio Sep 17, 2022

2opremio Sep 19, 2022

tomerweller commented Sep 17, 2022

leighmcculloch commented Sep 17, 2022

paulbellamy commented Sep 17, 2022 •

edited

paulbellamy Sep 17, 2022

leighmcculloch commented Sep 17, 2022 •

edited

paulbellamy commented Sep 17, 2022 •

edited

leighmcculloch commented Sep 17, 2022

leighmcculloch commented Sep 17, 2022 •

edited

2opremio commented Sep 19, 2022 •

edited

leighmcculloch commented Sep 19, 2022

2opremio commented Sep 19, 2022 •

edited

leighmcculloch commented Sep 19, 2022

leighmcculloch commented Sep 19, 2022 •

edited

	#[clap(long, parse(from_os_str))]
	#[clap(long, parse(from_os_str), default_value(".soroban/events.json"))]

invoke: add --event and --event-file flags #137

invoke: add --event and --event-file flags #137

Conversation

2opremio commented Sep 16, 2022 • edited

What

Why

Known limitations

leighmcculloch commented Sep 16, 2022 • edited

leighmcculloch Sep 16, 2022 • edited

Choose a reason for hiding this comment

2opremio Sep 17, 2022

Choose a reason for hiding this comment

2opremio Sep 19, 2022

Choose a reason for hiding this comment

tomerweller commented Sep 17, 2022

leighmcculloch commented Sep 17, 2022

paulbellamy commented Sep 17, 2022 • edited

paulbellamy Sep 17, 2022

Choose a reason for hiding this comment

leighmcculloch commented Sep 17, 2022 • edited

paulbellamy commented Sep 17, 2022 • edited

leighmcculloch commented Sep 17, 2022

leighmcculloch commented Sep 17, 2022 • edited

2opremio commented Sep 19, 2022 • edited

leighmcculloch commented Sep 19, 2022

2opremio commented Sep 19, 2022 • edited

leighmcculloch commented Sep 19, 2022

leighmcculloch commented Sep 19, 2022 • edited

2opremio commented Sep 16, 2022 •

edited

leighmcculloch commented Sep 16, 2022 •

edited

leighmcculloch Sep 16, 2022 •

edited

paulbellamy commented Sep 17, 2022 •

edited

leighmcculloch commented Sep 17, 2022 •

edited

paulbellamy commented Sep 17, 2022 •

edited

leighmcculloch commented Sep 17, 2022 •

edited

2opremio commented Sep 19, 2022 •

edited

2opremio commented Sep 19, 2022 •

edited

leighmcculloch commented Sep 19, 2022 •

edited