-
Notifications
You must be signed in to change notification settings - Fork 5
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
Session Token Testnet Faucet #12
Conversation
…it button is not rendered
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.
Nice work dude! A couple of changes required
apps/staking/app/faucet/actions.ts
Outdated
const transaction = db.transaction((discordExport: Array<DiscordIdExport>) => { | ||
for (const { id } of discordExport) { | ||
insert.run(BigInt(id).toString()); | ||
} | ||
}); |
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.
just had a quick look if batch insert could be a good idea, but I don't think it makes much of a difference if the inserts are already in a transaction, so all good
apps/staking/app/faucet/actions.ts
Outdated
const transaction = db.transaction((telegramExport: Array<TelegramIdExport>) => { | ||
for (const { id } of telegramExport) { | ||
insert.run(BigInt(id).toString()); | ||
} | ||
}); | ||
|
||
const faucetGasWarning = 0.01; | ||
const faucetSENTWarning = 20000; | ||
transaction(telegramExport); | ||
|
||
db.close(); |
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.
not sure how to read this, but maybe the db.close should be in a finally ?
https://github.com/WiseLibs/better-sqlite3/blob/master/docs/api.md#transactionfunction---function
it seems that transaction will rollback and then rethrow. Not sure
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.
I see, I've made changes to all the import functions to handle this
|
||
if (!operatorTableTimestampCutoff) return false; | ||
|
||
const lastUpdate = db |
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.
where do you update this OPERATOR_LAST_UPDATE field? By hand?
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.
do you not need that commented code at all?
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.
so i added this so we could use an endpoint to update this, but that endpoint doesnt exist yet
apps/staking/app/faucet/actions.ts
Outdated
const hoursBetweenTransactions = process.env.FAUCET_HOURS_BETWEEN_USES; | ||
const lastTransactionCutoff = hoursBetweenTransactions | ||
? Date.now() - parseInt(hoursBetweenTransactions) * 60 * 60 * 1000 | ||
: 0; |
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.
I think parseInt returns Nan if process.env.FAUCET_HOURS_BETWEEN_USES is not a number, maybe you should check for that
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.
fixed
apps/staking/app/faucet/actions.ts
Outdated
* @returns A boolean indicating whether a recent transaction exists. | ||
*/ | ||
function hasRecentTransaction({ db, source, id }: IdTableParams) { | ||
const hoursBetweenTransactions = process.env.FAUCET_HOURS_BETWEEN_USES; |
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.
same comment, maybe check that parseInt doesn't return Nan
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.
you should actually parse it on app start to a const , or throw
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.
Made changes
apps/staking/app/faucet/actions.ts
Outdated
async function updateNodeOperatorTable({ db }: { db: BetterSql3.Database }) { | ||
const nodeOperatorRes = await fetch('/oxen-swap'); | ||
|
||
if (!nodeOperatorRes.ok) { | ||
console.error('Failed to fetch node operator data'); | ||
return; | ||
} | ||
|
||
const { wallets } = (await nodeOperatorRes.json()) as NodeOperatorScoreResponse; | ||
|
||
const walletAddresses = Object.keys(wallets); | ||
|
||
const insert = db.prepare(`INSERT INTO ${TABLE.OPERATOR} (${OPERATOR_TABLE.ID}) VALUES (?)`); | ||
|
||
const transaction = db.transaction((walletAddresses: Array<string>) => { | ||
for (const address of walletAddresses) { | ||
insert.run(address); | ||
} | ||
}); | ||
|
||
transaction(walletAddresses); | ||
} | ||
*/ |
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.
I didn't review this code, let me know if we want it reviewed.
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.
All good
apps/staking/app/faucet/actions.ts
Outdated
/** | ||
* If the user has provided a Discord ID they must be in the approved list of Discord IDs and not have used the faucet recently. | ||
*/ | ||
if (discordId) { |
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.
if I read this right, those could be elseif
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.
true, made changes
apps/staking/app/faucet/actions.ts
Outdated
/** | ||
* If the user has provided a Telegram ID they must be in the approved list of Telegram IDs and not have used the faucet recently. | ||
*/ | ||
if (telegramId) { |
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.
same comment
if I read this right, those could be elseif
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.
true, made changes
|
||
if (hasRecentTransaction({ db, source: TRANSACTIONS_TABLE.DISCORD, id: discordId })) { | ||
throw new FaucetError( | ||
FAUCET_ERROR.ALREADY_USED_SERVICE, |
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.
you will be able to use this service a few times right (like every 24h or whatever). So maybe that string ALREADY_USED is misleading (as it seems like a one shot)
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.
so this is actually a way to identify the error thrown on the frontend so we can render a link in a localised error, so when we allow for x per hour we can just update the string. this error if you look in the network tab shows the full string and this short code.
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.
ok all good
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.
made changes
apps/staking/app/faucet/actions.ts
Outdated
const transaction = db.transaction((telegramExport: Array<TelegramIdExport>) => { | ||
for (const { id } of telegramExport) { | ||
insert.run(BigInt(id).toString()); | ||
} | ||
}); | ||
|
||
const faucetGasWarning = 0.01; | ||
const faucetSENTWarning = 20000; | ||
transaction(telegramExport); | ||
|
||
db.close(); |
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.
I see, I've made changes to all the import functions to handle this
|
||
if (hasRecentTransaction({ db, source: TRANSACTIONS_TABLE.DISCORD, id: discordId })) { | ||
throw new FaucetError( | ||
FAUCET_ERROR.ALREADY_USED_SERVICE, |
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.
so this is actually a way to identify the error thrown on the frontend so we can render a link in a localised error, so when we allow for x per hour we can just update the string. this error if you look in the network tab shows the full string and this short code.
apps/staking/app/faucet/actions.ts
Outdated
/** | ||
* If the user has provided a Telegram ID they must be in the approved list of Telegram IDs and not have used the faucet recently. | ||
*/ | ||
if (telegramId) { |
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.
true, made changes
apps/staking/app/faucet/actions.ts
Outdated
/** | ||
* If the user has provided a Discord ID they must be in the approved list of Discord IDs and not have used the faucet recently. | ||
*/ | ||
if (discordId) { |
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.
true, made changes
apps/staking/app/faucet/actions.ts
Outdated
source: TRANSACTIONS_TABLE.DISCORD | TRANSACTIONS_TABLE.TELEGRAM | TRANSACTIONS_TABLE.OPERATOR; | ||
id: string; | ||
}; | ||
|
||
/** | ||
* Checks if the given ID exists in the specified table of the database. | ||
* | ||
* @param db The database instance. | ||
* @param source The name of the table to search in. | ||
* @param id The ID to check for existence. | ||
* @returns A boolean indicating whether the ID exists in the table. | ||
*/ | ||
function idIsInTable({ db, source, id }: IdTableParams) { | ||
const field = 'id'; | ||
const row = db | ||
.prepare<string>(`SELECT count(${field}) FROM ${source} WHERE ${field} = ?`) | ||
.get(id) as CountType<typeof field>; |
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.
Ah good spot ive change it.
Yeah i wanted it to be generic as they do the same thing but i see your point. when we make changes to the faucet for the next stage of testnet I think im going to extract the database logic out of this file and then i can make specific calls for each table so we can make sure we have the right indexes
apps/staking/app/faucet/actions.ts
Outdated
* @returns A boolean indicating whether a recent transaction exists. | ||
*/ | ||
function hasRecentTransaction({ db, source, id }: IdTableParams) { | ||
const hoursBetweenTransactions = process.env.FAUCET_HOURS_BETWEEN_USES; |
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.
Made changes
apps/staking/app/faucet/actions.ts
Outdated
const hoursBetweenTransactions = process.env.FAUCET_HOURS_BETWEEN_USES; | ||
const lastTransactionCutoff = hoursBetweenTransactions | ||
? Date.now() - parseInt(hoursBetweenTransactions) * 60 * 60 * 1000 | ||
: 0; |
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.
fixed
|
||
if (!operatorTableTimestampCutoff) return false; | ||
|
||
const lastUpdate = db |
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.
so i added this so we could use an endpoint to update this, but that endpoint doesnt exist yet
apps/staking/app/faucet/actions.ts
Outdated
async function updateNodeOperatorTable({ db }: { db: BetterSql3.Database }) { | ||
const nodeOperatorRes = await fetch('/oxen-swap'); | ||
|
||
if (!nodeOperatorRes.ok) { | ||
console.error('Failed to fetch node operator data'); | ||
return; | ||
} | ||
|
||
const { wallets } = (await nodeOperatorRes.json()) as NodeOperatorScoreResponse; | ||
|
||
const walletAddresses = Object.keys(wallets); | ||
|
||
const insert = db.prepare(`INSERT INTO ${TABLE.OPERATOR} (${OPERATOR_TABLE.ID}) VALUES (?)`); | ||
|
||
const transaction = db.transaction((walletAddresses: Array<string>) => { | ||
for (const address of walletAddresses) { | ||
insert.run(address); | ||
} | ||
}); | ||
|
||
transaction(walletAddresses); | ||
} | ||
*/ |
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.
All good
Co-authored-by: Audric Ackermann <[email protected]>
|
||
const minTargetEthBalance = BigInt(FAUCET.MIN_ETH_BALANCE * Math.pow(10, ETH_DECIMALS)); | ||
|
||
const hoursBetweenTransactions = parseInt(process.env.FAUCET_HOURS_BETWEEN_USES ?? 0); |
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.
I guess this works except if FAUCET_HOURS_BETWEEN_USES
is set, but not a number.
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.
LGTM 🚢
No description provided.