-
Notifications
You must be signed in to change notification settings - Fork 155
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
dev-faucet #2061
dev-faucet #2061
Conversation
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.
Looks good so far, thank you for doing this!
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.
Some small nits but other than that looking good.
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.
This comment is a copy of the one above:
It kind of seems like we're doing a custom implementation of futures and an executor here? This is pretty impressive what you made here - but a lot of it also seems like it could be done using Rust's normal async primitives + an executor like Tokio/Block-on/Async-std. Right now we're calling a somewhat custom executor from within another we executor made, and that works, but I think we probably could've made it async native all the way through.
We're avoiding using the Tokio executor - but why exactly? It seems like there's a boilerplate could be avoided by functionality already existing in Tokio and other executors like spawned tasks, threads, and a ton of helper structs that provide things like queues that can work in an async context:
https://docs.rs/futures/0.3.21/futures/future/index.html
https://docs.rs/futures/0.3.21/futures/stream/index.html
I.e. like these for qeueing up tasks:
https://docs.rs/futures/0.3.21/futures/stream/struct.FuturesUnordered.html
or
https://docs.rs/futures/0.3.21/futures/stream/struct.FuturesOrdered.html
I think we lose some level of maintaibility here, ability to work with native rust concurrency, and also some pre-done optimization that other executors have.
If this already works and the faucet is ready to go, we can/maybe should go with this, but it could in the future potentially we could go towards a more "native async" approach.
Co-authored-by: Eran Rundstein <eran@rundste.in>
* make the faucet have a background thread that splits tx outs this enables it to support multiple faucet requests concurrently * fix clippies, clean up some things, add tracking for queue depth * bug fix and logging * fix lints * fix responses * fix documentation and examples * use async routes for compat with rocket 0.5 * Make more parameters configurable * re-order worker thread checks * skip serializing empty things * add readme to cargo toml
Co-authored-by: Mike Turner <zaphrod.beeblebrox@gmail.com>
Co-authored-by: Remoun Metyas <remoun@mobilecoin.com>
Co-authored-by: Remoun Metyas <remoun@mobilecoin.com>
Co-authored-by: Remoun Metyas <remoun@mobilecoin.com>
Co-authored-by: Remoun Metyas <remoun@mobilecoin.com>
Co-authored-by: Remoun Metyas <remoun@mobilecoin.com>
Co-authored-by: Remoun Metyas <remoun@mobilecoin.com>
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.
LGTM, although there might be an issue with mixing the grpcio calls and tokio executor. The recommended way according to some issues I found on the grpcio GH repository is to use some form of channel to bridge between the two.
With that said, we don't particularly care about slowing down the http request handling here so I think it is okay if they block for the duration of the mobilecoind GRPC calls.
Not blocking this on approval since this isn't critical code, but I do suggest documenting this and maybe not using the _async
call variants at all inside tokio async functions.
Co-authored-by: Remoun Metyas <remoun@mobilecoin.com>
This adds a thin HTTP server called
mobilecoind-dev-faucet
, similar tomobilecoind-json
but much smaller, which functions as a faucet and can be deployed in dev networks.This an alternative way that clients can get funds to run tests, which may simplify a lot of the things we do in deployment around funding a large number of accounts of several varieties as part of the deployment.
The server has a background thread which periodically checks if it should split off smaller TxOut's from the main faucet balance, and queues pre-split TxOut's in a tokio queue.
It should be able to handle a relatively high volume of concurrent faucet requests, so if e.g. SDK unit tests hit the faucet many times it won't fail.
I tested this version locally using the local_network.py script, it seems to be working as expected.