Skip to content
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

Merged
merged 43 commits into from
Jun 9, 2022
Merged

dev-faucet #2061

merged 43 commits into from
Jun 9, 2022

Conversation

cbeck88
Copy link
Contributor

@cbeck88 cbeck88 commented May 26, 2022

This adds a thin HTTP server called mobilecoind-dev-faucet, similar to mobilecoind-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.

@cbeck88 cbeck88 requested review from jcape and remoun May 26, 2022 23:59
@cbeck88 cbeck88 self-assigned this May 27, 2022
@cbeck88 cbeck88 requested review from eranrund and a team May 27, 2022 00:00
Copy link
Contributor

@eranrund eranrund left a 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!

mobilecoind-dev-faucet/README.md Outdated Show resolved Hide resolved
mobilecoind-dev-faucet/src/bin/main.rs Outdated Show resolved Hide resolved
@cbeck88 cbeck88 changed the title WIP dev-faucet dev-faucet May 27, 2022
@cbeck88 cbeck88 added the 1.3.0 label May 29, 2022
@jcape jcape added this to the 1.3.0 Release milestone May 31, 2022
@jcape jcape removed the 1.3.0 label May 31, 2022
Copy link
Contributor

@eranrund eranrund left a 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.

mobilecoind-dev-faucet/src/bin/main.rs Outdated Show resolved Hide resolved
mobilecoind-dev-faucet/src/bin/main.rs Outdated Show resolved Hide resolved
mobilecoind-dev-faucet/src/bin/main.rs Outdated Show resolved Hide resolved
mobilecoind-dev-faucet/src/data_types.rs Outdated Show resolved Hide resolved
mobilecoind-dev-faucet/src/lib.rs Outdated Show resolved Hide resolved
mobilecoind-dev-faucet/src/worker.rs Outdated Show resolved Hide resolved
mobilecoind-dev-faucet/src/worker.rs Outdated Show resolved Hide resolved
mobilecoind-dev-faucet/README.md Outdated Show resolved Hide resolved
mobilecoind-dev-faucet/README.md Outdated Show resolved Hide resolved
mobilecoind-dev-faucet/README.md Outdated Show resolved Hide resolved
mobilecoind-dev-faucet/README.md Outdated Show resolved Hide resolved
mobilecoind-dev-faucet/README.md Outdated Show resolved Hide resolved
mobilecoind-dev-faucet/README.md Outdated Show resolved Hide resolved
mobilecoind-dev-faucet/src/bin/main.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@iamalwaysuncomfortable iamalwaysuncomfortable left a 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.

mobilecoind-dev-faucet/src/worker.rs Show resolved Hide resolved
Base automatically changed from mobilecoind-last-block-info to master June 1, 2022 19:36
cbeck88 and others added 13 commits June 1, 2022 14:18
* 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
mobilecoind-dev-faucet/src/bin/main.rs Outdated Show resolved Hide resolved
mobilecoind-dev-faucet/src/data_types.rs Outdated Show resolved Hide resolved
mobilecoind-dev-faucet/src/lib.rs Outdated Show resolved Hide resolved
mobilecoind-dev-faucet/src/lib.rs Outdated Show resolved Hide resolved
mobilecoind-dev-faucet/src/lib.rs Outdated Show resolved Hide resolved
util/serial/src/lib.rs Outdated Show resolved Hide resolved
util/serial/src/lib.rs Show resolved Hide resolved
mobilecoind-dev-faucet/src/worker.rs Outdated Show resolved Hide resolved
mobilecoind-dev-faucet/src/worker.rs Outdated Show resolved Hide resolved
mobilecoind-dev-faucet/src/bin/main.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@eranrund eranrund left a 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.

mobilecoind-dev-faucet/src/data_types.rs Outdated Show resolved Hide resolved
mobilecoind-dev-faucet/src/lib.rs Show resolved Hide resolved
mobilecoind-dev-faucet/src/lib.rs Outdated Show resolved Hide resolved
@cbeck88 cbeck88 merged commit 8320c72 into master Jun 9, 2022
@cbeck88 cbeck88 deleted the dev-faucet branch June 9, 2022 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants