-
Notifications
You must be signed in to change notification settings - Fork 49
Proposed changes for multi-testnet faucet option #32
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,6 +36,8 @@ console.log('Acting as faucet for address:', config.address) | |
// | ||
|
||
// ProviderEngine based caching layer, with fallback to geth | ||
// | ||
// need to be able to dynamically change rpcOrigin based on network requesting funds | ||
const engine = rpcWrapperEngine({ | ||
rpcUrl: config.rpcOrigin, | ||
addressHex: config.address, | ||
|
@@ -85,6 +87,11 @@ function startServer () { | |
async function handleRequest (req, res) { | ||
try { | ||
// parse address | ||
// can parse for network here. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. server must have an explicit allowlist (not a denylist) for network ids it permits to send from. that faucet occasionally accidentally holds real world assets on mainnet and bsc There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kumavis Got it, what does that mean for the parsing here? |
||
// can either be json object or | ||
// inserted in a combined rawdata string with address, and then parsed out here | ||
// depending on what the network is, make sure ethQuery engine knows when | ||
// testing balance and sending transaction | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. go with json There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sounds good! |
||
let targetAddress = req.body | ||
if (!targetAddress || typeof targetAddress !== 'string') { | ||
return didError(res, new Error(`Address parse failure - request body empty`)) | ||
|
@@ -110,13 +117,16 @@ function startServer () { | |
const alignedIpAddress = ipAddress.padStart(15, ' ') | ||
const requestorMessage = `${flag} ${alignedIpAddress} requesting for ${targetAddress}` | ||
// check for greediness | ||
// | ||
// need to change network here: | ||
const balance = await ethQuery.getBalance(targetAddress, 'pending') | ||
const balanceTooFull = balance.gt(MAX_BALANCE) | ||
if (balanceTooFull) { | ||
console.log(`${requestorMessage} - already has too much ether`) | ||
return didError(res, new Error('User is greedy - already has too much ether')) | ||
} | ||
// send value | ||
// need ot change network here as well: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we're currently doing eip155 sigs here, we should fix that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@kumavis I don't think we will if we're using Infura the way it normally is. As you can see here, you can change the Ethereum network from an Infura API simply by changing the subdomain: The one issue I'm having is how to dynamically change Do you have any ideas of how to do this? |
||
const txHash = await ethQuery.sendTransaction({ | ||
to: targetAddress, | ||
from: config.address, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,6 +57,27 @@ function getNetwork () { | |
if (res.error) return console.res.error(res.error) | ||
var network = res.result | ||
state.network = network | ||
|
||
// set networkName for use in etherscan URL | ||
switch(network) { | ||
case '3': | ||
state.networkName = 'ropsten'; | ||
break | ||
case '42': | ||
state.networkName = 'kovan'; | ||
break | ||
case '4': | ||
state.networkName = 'rinkeby'; | ||
break | ||
case '5': | ||
state.networkName = 'goerli'; | ||
break | ||
// not sure about the best way to handle this, but | ||
// need to have something if they're on non-public testnet | ||
default: | ||
state.networkName = 'unknown'; | ||
break | ||
} | ||
renderApp() | ||
}) | ||
} | ||
|
@@ -209,7 +230,7 @@ function renderApp () { | |
} | ||
}, ( | ||
state.transactions.map((txHash) => { | ||
return link(`https://ropsten.etherscan.io/tx/${txHash}`, txHash) | ||
return link(`https://${state.networkName}.etherscan.io/tx/${txHash}`, txHash) | ||
}) | ||
)) | ||
]) | ||
|
@@ -232,16 +253,18 @@ async function getEther () { | |
if (!account) return | ||
|
||
var uri = `${window.location.href}v0/request` | ||
var data = account | ||
var data = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. may have to json serialize this first. verify that the fetch api supports providing an object There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kumavis just a quick search makes it seem like I may have to use |
||
'account' : account, | ||
'network' : state.network, | ||
} | ||
|
||
let res, body, err | ||
|
||
try { | ||
res = await fetch(uri, { | ||
method: 'POST', | ||
body: data, | ||
headers: { | ||
'Content-Type': 'application/rawdata' | ||
'Content-Type': 'application/json' | ||
} | ||
}) | ||
body = await res.text() | ||
|
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.
the provider stack is quite, old it should be replace by https://github.com/MetaMask/eth-json-rpc-middleware and the json-rpc-engine stack
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.
@kumavis Will that change the syntax here much?