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

[Faucet] Implement simple faucet service #1549

Merged
merged 13 commits into from
Apr 25, 2022
Merged

[Faucet] Implement simple faucet service #1549

merged 13 commits into from
Apr 25, 2022

Conversation

666lcz
Copy link
Contributor

@666lcz 666lcz commented Apr 22, 2022

Overview

This PR implements a modularized and extensible Faucet service with a naive Coin selection/splitting/transferring algorithm. The goal is to have something simple deployed to the Devnet first while I will improve concurrency in subsequent PRs.

Algorithm

  • The core algorithm is in src/faucet/simple_faucet.rs, which always uses two coins in the active address(one to split and sent to users, and the other to pay gas fee for operations such as splitting and transferring). It selects the two coin during initialization with highest balances.
  • For each request, we will send the recipient 5 coins with 2000 gas each(10000 in total)(see rationale here)
  • It assumes that during genesis, we will credit the active address owned by the faucet with an insanely high balance(e.g. u64 can handle around 32k requests, which should be enough for devnet)
  • Always using two coins is clearly not scalable since all operations need to be sequential. Subsequent PRs will improve the concurrency by using a queue of gas objects

Architecture

  • src/faucet contains logic for coin transferring/splitting
  • Previously I was planning to expose the service as part of the existing JSON-RPC server instead of a new HTTP Server. However, it's important that we have access to the IP address of each request in order to do rate-limitting(not rate-limiting by number of network requests from each IP, but rate-limiting by number of gas coins transferred). But the JSON-RPC library we are using does not expose IP address. In addition, setting up a separate server makes it very easy for us to add new user-facing features

Next steps

  • Deploy to the Devnet
  • Add a wallet command for requesting gas
  • Implement rate-limiting by IP and Sui address
  • Improve concurrency

Testing

Added unit tests. I also did an end to end testing as below:

curl --location --request POST 'http://127.0.0.1:5003/gas' \
--header 'Content-Type: application/json' \
--data-raw '{
    "FixedAmountRequest": {
        "recipient": "D319FF9E39F619823D7B7D1F6E896B817AA1B113"
    }
}'

Response

{
    "transferred_gas_objects": [
        {
            "amount": 20,
            "id": "3d644dd136ea5cbb1340274de71155eb97ee5db5"
        },
        {
            "amount": 20,
            "id": "58e9ed7e78bae97a660dfd2a50ed6fb7c31ca8c1"
        },
        {
            "amount": 20,
            "id": "62ff612f693e73cab41ec3497ab51e8754a3e4fe"
        },
        {
            "amount": 20,
            "id": "939f7cefc7852def869582aee280f193420055ff"
        },
        {
            "amount": 20,
            "id": "e108dc836f010b4849268f2d9502116ecc0692a9"
        }
    ],
    "error": null
}

Used the wallet CLI to verify that the address indeed received these coins

wallet gas --address D319FF9E39F619823D7B7D1F6E896B817AA1B113
                Object ID                 |  Version   |  Gas Value
----------------------------------------------------------------------
 3CD35E0D65C5B5995C940106D1F4FB8CAF5025F7 |     0      |   100000
 3D644DD136EA5CBB1340274DE71155EB97EE5DB5 |     2      |     20
 536EA9BE19570114FEBB57890628E9C1C70FF1D8 |     0      |   100000
 58E9ED7E78BAE97A660DFD2A50ED6FB7C31CA8C1 |     2      |     20
 62FF612F693E73CAB41EC3497AB51E8754A3E4FE |     2      |     20
 939F7CEFC7852DEF869582AEE280F193420055FF |     2      |     20
 D045DD52E11A66BC38ACA068F0608617A525FA61 |     0      |   100000
 E108DC836F010B4849268F2D9502116ECC0692A9 |     2      |     20
 EAC90BCFE43192F3CB3B759B113285AD6F6FE114 |     0      |   100000
 ECDE9B62E605716C65AEC6A031DFE513E3BA26CD |     0      |   100000

@666lcz 666lcz requested review from lxfind, longbowlu and velvia April 22, 2022 23:55
@666lcz
Copy link
Contributor Author

666lcz commented Apr 24, 2022

Addressed @bmwill and @patrickkuo 's comments

  • removed the service layer
  • removed Arc<Mutex<>>
  • removed Send/Sync
  • Renamed server.rs -> main.rs

Copy link
Contributor

@bmwill bmwill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making those changes to simplify this!

Comment on lines +28 to +31
struct AppState<F = SimpleFaucet> {
faucet: F,
// TODO: add counter
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reasoning for wanting this to be generic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case I want to experiment with a different implementation of Faucet(e.g., using a queue of gas objects,etc) for some traffic and can quickly switch to another one in case the more complicated implementation is failing.

ServiceBuilder::new()
.layer(HandleErrorLayer::new(handle_error))
.buffer(REQUEST_BUFFER_SIZE)
.concurrency_limit(CONCURRENCY_LIMIT)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have context on how the internal wallet handles concurrent access so we may just want to be careful and sure that this service still functions properly with parallel requests (although the concurrency_limit should act as that bottleneck)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the current implementation only work for CONCURRENCY_LIMIT = 1 but will relax this assumption in a subsequent PR

@oxade
Copy link
Contributor

oxade commented Apr 25, 2022

@666lcz thanks for handling this

@666lcz 666lcz merged commit 0a29d0d into main Apr 25, 2022
@666lcz 666lcz deleted the chris/faucet-0 branch April 25, 2022 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants