-
Notifications
You must be signed in to change notification settings - Fork 76
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
feat: add cache sub commands and allow for proper transaction data logging #1173
Conversation
f721edd
to
d1e5709
Compare
Currently this requires: stellar/stellar-rpc#90 |
fb6b4f5
to
b7a66d5
Compare
b7a66d5
to
9b4aa23
Compare
To be able to print, as well as save to a local file |
fbdf760
to
1478548
Compare
9462ba6
to
83d7b93
Compare
8384fc5
to
15c30f5
Compare
15c30f5
to
2c8afda
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few comments inline.
If I understand correctly this PR serves a couple purposes:
-
Caching contract specs so they don't need to be downloaded repeatedly. 💯 👍🏻
-
Caching transactions and simulations to facilitate tracking fees and resource consumption in the CLI (soroban-cli: automatically save resource cost of transactions #1106). My understanding of soroban-cli: automatically save resource cost of transactions #1106 was a bulk way of collecting statistics-like data, data that folks could feel comfortable sharing, but caching full transactions doesn't feel exactly aligned with that, as it's storing individual transactions and making individual transactions retrievable from the cache, and folks may not want to share individual transactions with all the arguments, auths, signatures, etc, not to mention their size will be unnecessarily large. @namankumar I think we might need a user story drawn up for your issue to make sure that what we're building here fits the user workflow. What format and what data exactly were you hoping to get with soroban-cli: automatically save resource cost of transactions #1106? I think storing a tx resource log that stores tx id, resource details, fee details would probably suffice?
While the intent of #1106 was, like Leigh said, to reduce developer effort in profiling their transactions and sharing the aggregate data, I wonder whether having a full cache of tx is actually a net benefit? It can allow a developer to review their historical tx as they fine tune the code for limits and fees. To be clear, I still only need the aggregate data and not the full tx; but now that the cacheing code is written, perhaps we can give it shape to make a useful feature out of it. @janewang thoughts? Just an idea |
08d62be
to
ed02780
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple minor things I commented inline that can be fixed up when merging.
A bigger issue is that the commands don't tell a clear story.
The cache stores contract specs, but the cache ls
and cache read
commands don't display contract specs that have been cached.
The cache stores responses to requests to rpc, and they're visible in the cache ls
and cache read
commands, but they aren't used as a cache, they're used as a log, and the cache ls
and cache read
commands are accessing that log.
In terms of what should be in the "cache", if we're true to what a cache is and is used for, it should just be the contract specs, and the request/response log should be a separate thing.
I'm inclined to merge just the contract spec caching parts of this PR, simplify the commands to only the clean command. Then we aren't merging commands that we don't really have a goal for that would then be a breaking change later to remove or fixup. We can always circle back to reviving this logic out of the PR at a later date if needed.
@janewang Does that work for you?
Thanks for flagging. I'm filling in for @janewang here. Could we simply add a new command "log"? |
The term log on its own is overloaded. But yes we can move the actions log / request log items into their own command relatively simply. |
Actually a better story is making both subcommands of "cache", which opens space for future development as well. cache specs |
I like it. Id change it to: cache clean // cleans everything Basically move the commands that only work for txs into their own. We don't need any fns for spec at the moment. And clean can work for all. I'm torn on the commandlog vs say requestlog. |
Sounds great! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a few minor changes (couple bug fixes, removed some incomplete functionality around output types, and changed the output type to be the full cached data instead of a subset of it).
In the end after testing this in a variety of situations I'm unclear on how useful the caching of the contract specs are actually going to be until we speed up the rpc. While it means the WASM doesn't have to be repeatedly downloaded, we still check which wasm the contract uses, and the RPC is pretty slow at providing that response, so the difference in user experience is negligible. But that's something we can improve on. If the RPC can get faster, then the whole thing will be faster. [Edit: I realised I'm testing from a far distance from the RPC I was testing with, so it's going to be delayed because of that so probably not a good test.]
@namankumar The actionslog command I left as is rather than split it up as you suggested only because this commands future is dubious at best and if we want to make a thing of it we should come up with a good story for it. I marked the command as experimental with a help comment so that folks know this is not a stable feature and will change.
fixes #1106
Adds the following cache subcommands:
This PR also caches referenced contracts dramatically speeding up using
invoke
and --help