Feature/typescript xrpl.js #42
Conversation
|
Have you tested this on a standalone rippled instance with a high networkId? |
src/config.ts
Outdated
| @@ -0,0 +1,57 @@ | |||
| export interface Config { | |||
| NODE_ENV: "production" | "development" | |||
| PORT: string | |||
There was a problem hiding this comment.
Should the values that have "defaults" later on be optional here?
Also I'd recommend grouping the "optional" fields at the end, and giving them a comment above like '// Optional - See "defaults" for default valuesto clarify that they do have to be in the final object, but maybe not the.config` file.
Maybe instead of having one type, Config should be the type passed around in the code, and ConfigFile should represent the type that users provide, which is used to generate the full Config object.
src/config.ts
Outdated
| NODE_ENV: "production" | "development" | ||
| PORT: string | ||
| RIPPLED_URI: string | ||
| FUNDING_ADDRESS: string |
There was a problem hiding this comment.
FUNDING_ADDRESS doesnt seem to be used. I made a mistake in not including this as part of the initial rewrite. It should be used to cover the case that the faucet account is rekeyed and the secret is now different than the original one to create the account. This existed in the previous version but I stupidly deleted it.
There was a problem hiding this comment.
where in the previous version does it exist? can't seem to find it
There was a problem hiding this comment.
It uses it here https://github.com/xpring-eng/testnet-faucet/blob/master/index.js#L173. By it being used this way though it requires you to always specify both properties.
I think it would be better since we are using the wallet class to supply it there.
There was a problem hiding this comment.
fromSeed(FUNDING_SEED, { masterAddress: FUNDING_ADDRESS })
| export default async function (req: Request, res: Response) { | ||
| let client = await getConnectedClient(); | ||
| try { | ||
| const serverInfo: ServerInfoResponse = await client.request({ |
There was a problem hiding this comment.
nit: server_info, fee and account_info could all happen at the same time via a Promise.all
| ledger_index: "current", | ||
| queue: true, | ||
| }); | ||
| console.log( |
There was a problem hiding this comment.
nit: Use template strings to clean up these logging statements
| 140, | ||
| config.MAX_TICKET_COUNT - ticketQueue.length | ||
| ); | ||
| console.log( |
There was a problem hiding this comment.
nit: Should we add request id to these logs to keep track of the request that necessitated getting more tickets?
package.json
Outdated
| @@ -2,18 +2,27 @@ | |||
| "name": "testnet-faucet", | |||
| "version": "1.0.0", | |||
There was a problem hiding this comment.
Should bump the version of this to 2.0.0
Co-authored-by: Jackson Mills <[email protected]>
High Level Overview of Change
upgrade faucet code to typescript and using xrpl.js V2, use tickets to arrange and track sequence numbers.
break up routes into separate files and clean up code.
Context of Change
CBDC team pointed out the existing faucet doesn't work with custom networkIds and will need an update to support them
Type of Change