keep-test: ETH faucet #174

sthompson22 · 2019-11-25T18:34:56Z

Intro

Sending ETH around is a pain in the ass. To try and help ease that burden a bit we want to make it so folks can fund ETH accounts via Heimdall.

Here we provide the first iteration of this with a command that will fund an ETH account that exists in the keep-test environment. We chose keep-test because it's active for tBTC testing, this should make it easier for devs where they need it most.

How it works

eth-faucet fund <target account>

fund is the only command that exists at the moment.

Testing

closes: #176

In order for the freshly minted keep-test-eth-faucet Heimdall script to run we need web3 commands available. Here we install that package.

We run a handful of internal ethereum chains, they're a pain in the ass to fund accounts on. You have to use the geth console, or geth CLI to transfer eth. Our keep-test environment has a public transaction node. This means we can take advantage of Heimdall to fund accounts on this chain, instead of having to use the aforementioned methods. Here we provide a very basic Heimdall command that funds a passed account with 10 eth.

Can go either way really...but we need to whole-ass whichever way that is.

kb0rg · 2019-11-25T22:12:34Z

I haven't dug into the code yet, but I do have one minor procedural note: we're using a pre-commit hook for linting with prettier. This repo tracks the related .pre-commit-config.yaml and .prettierrc.yaml files, but you'll need to run pre-commit install to set up your hook

(This should probable be added to the project's README, sorry 😬 -- feel free to add it if you're inspired)

kb0rg

Hi hi, I know this is still in draft, so apologies if you weren't looking for notes yet (I wasn't sure if the 👀 were an invitation for notes or just a peek).

I'm still trying to wrap my head around what needs to be done to fix the log ordering issue discussed here.

In the meantime I wanted to add a small note or two.

kb0rg · 2019-11-26T00:30:34Z

scripts/keep-test-eth-faucet.js

+
+const web3 = new Web3(new Web3.providers.HttpProvider(ethHost + ':' + ethRpcPort), null, web3_options);
+
+module.exports = async function(robot) {


I don't think you want the async here at the module.export, only within the definitions of the functions for the commands themselves.

I may be wrong on this -- tbh I don't know if it will hurt anything -- but I don't see a precedent for it in our other scripts. (But please correct me if you had a reason for putting it here!)

I want to say I tried this and ran into an error regarding await only being usable in async functions. Will test again.

^ was wrong, works fine w/out async

kb0rg · 2019-11-26T03:09:14Z

scripts/keep-test-eth-faucet.js

+      msg.send(`Account ${purse} unlocked!`);
+    } catch (error) {
+        robot.logger.error(`ETH account unlock error: ${error.message}`);
+        msg.send('There was an issue unlocking the purse account, ask for an adult!');


ask for an adult

lolololololol

kb0rg · 2019-11-28T01:31:23Z

I wanted to follow up here with a couple notes I left in flowdock, so you'd have review-related conversations all in one place:

The thread in Bifrost, where you test this demonstrates an issue with the logging showing up out of order. I haven't 100% sorted out why, but discuss a couple thoughts in that thread. I suspect the key is in the examples of sequentialStart vs concurrentStart here
There's some helpful info here on how to deal with the npm/ python errors you're getting

Rather than peg the script to an environment we're making the naming generic, with a note that the script only works for keep-test at the moment. This is to set the intention that we should be able to use this script for multiple environments, it just needs to be extended.

We were using mixed mode variable expansion in strings, I updated the one outlier to match the rest of the script. Also added a note that this script only works with keep-test at the moment.

Eventually we'll be able to pass an environment as an argument to this command. For now we shorten and make the faucet invocation command generic, as it should be.

Frankly I'm not sure why we can do this. In previous experience you had to use async/await all the way up the path, else you would run into runtime errors regarding using await in a non-async function.

It's a bit unnecessary from the perspective of the person being funded. They don't care where the ETH comes from, and we as operators know who the purse is.

In an effor to enforce ordering we wrap the msg.send calls for await results in then. This seems to work, however it produces a new scenario where then then log is returned before the msg.send message that should fire BEFORE the await. Need to look closer.

we need to exit on catch, adding a return so that if we catch we don't try to continue the script.

The "account" in this case is the purse. We should clarify that this password is for the purse, not just an eth account.

tl;dr here is that we think the ordering issues on Flowdock are due to the fact that the unlocked and funding msg.sends are firing at about the same time, and that Flowdock doesn't guarantee ordering in that case. We're just going to go ahead and remove the unlocked log, as this falls over if the accounnt isn't unlocked anyway (and we have logging for that).

Needed to update the usage comment after shortening the command. This also includes changes made by prettier.

sthompson22 · 2019-12-03T21:19:59Z

I haven't dug into the code yet, but I do have one minor procedural note: we're using a pre-commit hook for linting with prettier. This repo tracks the related .pre-commit-config.yaml and .prettierrc.yaml files, but you'll need to run pre-commit install to set up your hook

Done.

sthompson22 · 2019-12-03T21:54:27Z

[Update: ohhhhh. trying to resolve the python noise? Have you looked at the link in the second point here? There are a couple suggestions in that old GH issue that might help]

👀

sthompson22 · 2019-12-03T23:03:39Z

We got there.

kb0rg

This has been fun to watch evolve, excited to get it live so folks can use it!

kb0rg · 2019-12-18T23:00:09Z

@sthompson22 --
There's one thing I wondered about when I first reviewed this PR -- it didn't seem worth investigating/ blocking release at the time, but curiosity nags at me, so I've still been wondering...

The addition of web3 has inflated our npm packages quite a bit.

It's not a significant time hit on the install time (1.323s to 2.178s, on my laptop), but as the install logs scroll by, it's clear there's a LOT more going on now.

Even though only (heh, only) 236 packages were added, that translates to a huge increase of the number of packages audited (from 1038 packages to 22883).

The web3 package is an umbrella package that includes a few smaller modules, which can be installed separately.

I haven't dug deep enough to know whether or not that's possible in this case.

At first glance, it looks like maybe we could switch to importing web3-eth and web3-providers-http and web3-utils separately. But I have no idea whether they would still end up installing the same number of dependencies anyway (or if this is even worth investigating, since it doesn't impact the build time that much)....

Wanted to run the thought by you though and see what you think?

kb0rg · 2019-12-20T20:21:40Z

(Following up here after chatting about the previous comment in a call with @sthompson22 )

Sloan will look into the above, to see if it is doable/ if it helps, but there's no urgency around it at all.

Also, he noticed (and I missed in review) that his help text isn't consistent with our other commands. We discussed the existing conventions, and that the help should change from eth-faucet [fund] "ETH account address"
to
eth-faucet fund <ETH account address> Context here about what the command does

Sloan is planning an enhancement to this script, and will update when he works on that :)

Sloan Thompson added 5 commits November 23, 2019 06:23

Include web3 in Heimdall packages

4222fa9

In order for the freshly minted keep-test-eth-faucet Heimdall script to run we need web3 commands available. Here we install that package.

ily

36d6d89

To semicolon or not to semicolon

6279ea1

Can go either way really...but we need to whole-ass whichever way that is.

Some space preese

788915e

kb0rg reviewed Nov 26, 2019

View reviewed changes

Sloan Thompson added 10 commits December 3, 2019 08:25

Standardize variable expansion

3bf25da

We were using mixed mode variable expansion in strings, I updated the one outlier to match the rest of the script. Also added a note that this script only works with keep-test at the moment.

Shortened faucet command

23575ca

Eventually we'll be able to pass an environment as an argument to this command. For now we shorten and make the faucet invocation command generic, as it should be.

Remove async from from function(robot)

025147d

Frankly I'm not sure why we can do this. In previous experience you had to use async/await all the way up the path, else you would run into runtime errors regarding using await in a non-async function.

Removed purse account from funding log

e656363

It's a bit unnecessary from the perspective of the person being funded. They don't care where the ETH comes from, and we as operators know who the purse is.

Wrap success logs in then()

f067c04

In an effor to enforce ordering we wrap the msg.send calls for await results in then. This seems to work, however it produces a new scenario where then then log is returned before the msg.send message that should fire BEFORE the await. Need to look closer.

Return msg.send in catch blocks

3d51ab8

we need to exit on catch, adding a return so that if we catch we don't try to continue the script.

Rename ethAccountPassword

b3e4238

The "account" in this case is the purse. We should clarify that this password is for the purse, not just an eth account.

Update usage comment

7abcdc6

Needed to update the usage comment after shortening the command. This also includes changes made by prettier.

Sloan Thompson added 2 commits December 3, 2019 16:35

Stab stab

f83eff4

Last stab then I'm digging, does this one have all the things

c1ff9aa

Adding python dependencies due to web3

8037253

sthompson22 marked this pull request as ready for review December 3, 2019 23:03

sthompson22 assigned kb0rg Dec 4, 2019

kb0rg approved these changes Dec 4, 2019

View reviewed changes

kb0rg merged commit ad3e306 into master Dec 4, 2019

kb0rg deleted the sthompson22/keep-test/eth-faucet branch December 4, 2019 19:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

keep-test: ETH faucet #174

keep-test: ETH faucet #174

sthompson22 commented Nov 25, 2019 •

edited

Loading

kb0rg commented Nov 25, 2019 •

edited

Loading

kb0rg left a comment

kb0rg Nov 26, 2019

sthompson22 Dec 2, 2019

sthompson22 Dec 3, 2019

kb0rg Nov 26, 2019

kb0rg commented Nov 28, 2019

sthompson22 commented Dec 3, 2019

sthompson22 commented Dec 3, 2019

sthompson22 commented Dec 3, 2019

kb0rg left a comment

kb0rg commented Dec 18, 2019

kb0rg commented Dec 20, 2019


		const web3 = new Web3(new Web3.providers.HttpProvider(ethHost + ':' + ethRpcPort), null, web3_options);

		module.exports = async function(robot) {

keep-test: ETH faucet #174

keep-test: ETH faucet #174

Conversation

sthompson22 commented Nov 25, 2019 • edited Loading

Intro

How it works

Testing

kb0rg commented Nov 25, 2019 • edited Loading

kb0rg left a comment

Choose a reason for hiding this comment

kb0rg Nov 26, 2019

Choose a reason for hiding this comment

sthompson22 Dec 2, 2019

Choose a reason for hiding this comment

sthompson22 Dec 3, 2019

Choose a reason for hiding this comment

kb0rg Nov 26, 2019

Choose a reason for hiding this comment

kb0rg commented Nov 28, 2019

sthompson22 commented Dec 3, 2019

sthompson22 commented Dec 3, 2019

sthompson22 commented Dec 3, 2019

kb0rg left a comment

Choose a reason for hiding this comment

kb0rg commented Dec 18, 2019

kb0rg commented Dec 20, 2019

sthompson22 commented Nov 25, 2019 •

edited

Loading

kb0rg commented Nov 25, 2019 •

edited

Loading