feat: add optional automatic ERC20 transfer mode - #170
Conversation
📝 WalkthroughWalkthroughAdds an optional 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
tests/generate-payout-permit.test.ts (2)
58-67: ⚡ Quick winAlign the mocked transfer payload with the actual reward contract.
The mock and assertion omit
tokenTypeandowner, which are part of the transfer reward shape returned bysrc/handlers/generate-erc20-transfer.ts. Including them will catch contract drift earlier.Suggested patch
(generateErc20Transfer as jest.Mock).mockReturnValue({ type: "erc20-transfer", + tokenType: "ERC20", tokenAddress: "TOKEN_ADDRESS", beneficiary: SPENDER, amount: "100", + owner: "0x0000000000000000000000000000000000000001", networkId: 1, transactionHash: "0xabc", gasEstimate: "21000", feeTransfers: [], }); ... expect(result).toMatchObject([ { type: "erc20-transfer", + tokenType: "ERC20", tokenAddress: "TOKEN_ADDRESS", beneficiary: SPENDER, amount: "100", + owner: "0x0000000000000000000000000000000000000001", networkId: 1, transactionHash: "0xabc", gasEstimate: "21000", feeTransfers: [], }, ]);Also applies to: 113-124
127-136: ⚡ Quick winAdd guard-path tests for fee and gas-buffer validation.
These tests only verify happy paths. Add assertions for invalid fee bps (
-1,10001) and negative gas buffer to lock in the safety behavior.Suggested patch
it("should split operator fee from direct transfer amount", () => { expect(splitTransferAmount("1000", 250)).toEqual({ beneficiaryAmount: "975", operatorFeeAmount: "25", }); }); + it("should reject invalid operator fee bps", () => { + expect(() => splitTransferAmount("1000", -1)).toThrow("between 0 and 10000"); + expect(() => splitTransferAmount("1000", 10001)).toThrow("between 0 and 10000"); + }); + it("should add a configurable gas buffer to direct transfer estimates", () => { expect(addGasBuffer("21000", 2000).toString()).toBe("25200"); }); + + it("should reject negative gas buffer bps", () => { + expect(() => addGasBuffer("21000", -1)).toThrow("must not be negative"); + });
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b04e8ee2-3da7-4650-9f1c-627fae6b742d
📒 Files selected for processing (8)
SUBMISSION_NOTE.mdsrc/handlers/generate-erc20-transfer.tssrc/handlers/generate-payout-permit.tssrc/handlers/index.tssrc/types/env.tssrc/types/permits.tssrc/types/plugin-input.tstests/generate-payout-permit.test.ts
| const feeBasisPoints = parseFeeBps(feeBps); | ||
| const { beneficiaryAmount, operatorFeeAmount } = splitTransferAmount(grossAmount, feeBasisPoints); | ||
|
|
||
| const beneficiaryTransfer = await sendTokenTransfer(tokenContract, walletAddress, beneficiaryAmount); | ||
| const feeTransfers = []; | ||
|
|
||
| if (!BigNumber.from(operatorFeeAmount).isZero()) { | ||
| if (!feeRecipient) { | ||
| throw new Error("UBIQUITY_FEE_RECIPIENT must be configured when UBIQUITY_FEE_BPS is greater than zero"); | ||
| } | ||
| const resolvedFeeRecipient = await resolveTransferAddress(provider, feeRecipient); | ||
| const feeTransfer = await sendTokenTransfer(tokenContract, resolvedFeeRecipient, operatorFeeAmount); |
There was a problem hiding this comment.
Validate fee config before sending the beneficiary transfer.
With UBIQUITY_FEE_BPS > 0 and no/invalid recipient, Line 105 sends the reduced beneficiary amount first, then Lines 109-113 throw. That creates an irreversible partial payout.
Proposed fix
const grossAmount = utils.parseUnits(amount.toString(), tokenDecimals);
const feeBasisPoints = parseFeeBps(feeBps);
+ let resolvedFeeRecipient: string | undefined;
+ if (feeBasisPoints > 0) {
+ if (!feeRecipient) {
+ throw new Error("UBIQUITY_FEE_RECIPIENT must be configured when UBIQUITY_FEE_BPS is greater than zero");
+ }
+ resolvedFeeRecipient = await resolveTransferAddress(provider, feeRecipient);
+ }
const { beneficiaryAmount, operatorFeeAmount } = splitTransferAmount(grossAmount, feeBasisPoints);
const beneficiaryTransfer = await sendTokenTransfer(tokenContract, walletAddress, beneficiaryAmount);
const feeTransfers = [];
if (!BigNumber.from(operatorFeeAmount).isZero()) {
- if (!feeRecipient) {
- throw new Error("UBIQUITY_FEE_RECIPIENT must be configured when UBIQUITY_FEE_BPS is greater than zero");
- }
- const resolvedFeeRecipient = await resolveTransferAddress(provider, feeRecipient);
+ if (!resolvedFeeRecipient) {
+ throw new Error("UBIQUITY_FEE_RECIPIENT must be configured when UBIQUITY_FEE_BPS is greater than zero");
+ }
const feeTransfer = await sendTokenTransfer(tokenContract, resolvedFeeRecipient, operatorFeeAmount);| const parsed = Number.parseInt(feeBps, 10); | ||
| if (!Number.isFinite(parsed) || parsed < 0 || parsed > BASIS_POINTS) { | ||
| throw new Error("UBIQUITY_FEE_BPS must be an integer between 0 and 10000"); | ||
| } | ||
|
|
||
| return parsed; |
There was a problem hiding this comment.
Reject malformed fee bps strings exactly.
parseInt accepts values like "25abc" or "25.9" as 25, so a bad secret can silently change fee collection.
Proposed fix
- const parsed = Number.parseInt(feeBps, 10);
- if (!Number.isFinite(parsed) || parsed < 0 || parsed > BASIS_POINTS) {
+ if (!/^\d+$/.test(feeBps)) {
+ throw new Error("UBIQUITY_FEE_BPS must be an integer between 0 and 10000");
+ }
+
+ const parsed = Number(feeBps);
+ if (!Number.isSafeInteger(parsed) || parsed < 0 || parsed > BASIS_POINTS) {
throw new Error("UBIQUITY_FEE_BPS must be an integer between 0 and 10000");
}| async function sendTokenTransfer(tokenContract: Contract, beneficiary: string, amount: string) { | ||
| const gasEstimate = await tokenContract.estimateGas.transfer(beneficiary, amount); | ||
| const gasLimit = addGasBuffer(gasEstimate); | ||
| const transaction = await tokenContract.transfer(beneficiary, amount, { gasLimit }); | ||
|
|
||
| return { | ||
| transactionHash: transaction.hash, | ||
| gasEstimate: gasEstimate.toString(), | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/handlers/generate-erc20-transfer.ts | head -200 | tail -30Repository: ubiquity-os/permit-generation
Length of output: 1346
🏁 Script executed:
# Search for the function context
rg -A 20 "async function sendTokenTransfer" src/handlers/generate-erc20-transfer.tsRepository: ubiquity-os/permit-generation
Length of output: 830
🏁 Script executed:
# Check for any wait() calls in the file to understand current patterns
rg "\.wait\(" src/handlers/generate-erc20-transfer.tsRepository: ubiquity-os/permit-generation
Length of output: 55
🏁 Script executed:
# Check the imports to understand which Contract type is used
head -30 src/handlers/generate-erc20-transfer.tsRepository: ubiquity-os/permit-generation
Length of output: 1174
🌐 Web query:
ethers.js Contract transfer() method does it wait for confirmation
💡 Result:
No, the ethers.js contract transfer method does not automatically wait for confirmation [1][2]. When you call a state-changing method like transfer on a contract instance connected to a Signer, ethers.js sends the transaction to the network and immediately returns a ContractTransactionResponse object (a promise that resolves to the transaction response) [1][3][4]. At this stage, the transaction has been submitted to the mempool but has not yet been mined or included in a block [1][2]. To wait for the transaction to be mined or to reach a specific number of confirmations, you must explicitly call the.wait method on the returned transaction object [1][3][5]. Example pattern: // Send the transaction (returns a TransactionResponse immediately) const tx = await contract.transfer(toAddress, amount); // Wait for the transaction to be included in a block (default: 1 confirmation) const receipt = await tx.wait; If you pass a number to.wait(confirms), the method will resolve once the transaction has been mined and has the specified number of confirmations [3][5].
Citations:
- 1: https://docs.ethers.io/v6/single-page/
- 2: https://docs.ethers.org/v6/getting-started/
- 3: https://docs.ethers.org/v6/api/contract/
- 4: https://docs.ethers.org/v5/api/contract/contract/
- 5: https://docs.ethers.org/v4/cookbook-contracts.html
🏁 Script executed:
# Find calls to sendTokenTransfer to understand how the return value is used
rg "sendTokenTransfer" src/handlers/generate-erc20-transfer.ts -B 2 -A 2Repository: ubiquity-os/permit-generation
Length of output: 807
🏁 Script executed:
# Check if there's any fee transfer logic after the beneficiary transfer
rg -A 50 "async function sendTokenTransfer" src/handlers/generate-erc20-transfer.ts | head -80Repository: ubiquity-os/permit-generation
Length of output: 858
🏁 Script executed:
# Look for how transactionHash is used after being returned
rg "transactionHash" src/handlers/generate-erc20-transfer.ts -B 2 -A 2Repository: ubiquity-os/permit-generation
Length of output: 541
🏁 Script executed:
# Check if there's any code that depends on the success of sendTokenTransfer before checking finality
rg -B 5 -A 10 "feeRecipient\|feeBps" src/handlers/generate-erc20-transfer.ts | head -100Repository: ubiquity-os/permit-generation
Length of output: 55
Add transaction receipt check before reporting success.
tokenContract.transfer(...) broadcasts the transaction but doesn't wait for mining. Without explicit confirmation, a reverted or dropped transaction is reported as successful, and fee transfers run against unconfirmed state.
Proposed fix
const gasEstimate = await tokenContract.estimateGas.transfer(beneficiary, amount);
const gasLimit = addGasBuffer(gasEstimate);
const transaction = await tokenContract.transfer(beneficiary, amount, { gasLimit });
+ const receipt = await transaction.wait(1);
+ if (receipt.status !== 1) {
+ throw new Error(`ERC20 transfer failed: ${transaction.hash}`);
+ }
return {
- transactionHash: transaction.hash,
+ transactionHash: receipt.transactionHash,
gasEstimate: gasEstimate.toString(),
};| permit = context.config.transfer | ||
| ? await generateErc20Transfer(context, username, amount, tokenAddress) | ||
| : await generateErc20PermitSignature(context, username, amount, tokenAddress); |
There was a problem hiding this comment.
Add idempotency before executing direct transfers.
This branch performs irreversible ERC20 sends during payout generation. If a later request, fee transfer, or kernel callback fails, rerunning the job can duplicate already-sent payouts.
Use a persisted idempotency key per payout before calling generateErc20Transfer, and skip/return the existing transaction when it was already completed.
1 participant
Resolves #6
Summary
transfer: truesetting that routes ERC20 payout requests through a direct token transfer path instead of permit signature generation.generateErc20Transfer, which resolves the beneficiary wallet from the request username, decrypts the configured admin wallet, reads token decimals, estimates transfer gas, applies a 20% gas buffer, and sends the ERC20 transfer.UBIQUITY_FEE_BPSandUBIQUITY_FEE_RECIPIENT.transferis not enabled.Safety And Edge Cases
Verification
bun x jest tests/generate-payout-permit.test.ts --runInBandbun x jest --runInBandbun run buildbun x prettier --check src/types/plugin-input.ts src/types/env.ts src/types/permits.ts src/handlers/generate-payout-permit.ts src/handlers/index.ts src/handlers/generate-erc20-transfer.ts tests/generate-payout-permit.test.tsbun x eslint src/types/plugin-input.ts src/types/env.ts src/types/permits.ts src/handlers/generate-payout-permit.ts src/handlers/index.ts src/handlers/generate-erc20-transfer.ts tests/generate-payout-permit.test.tsbun x cspell src/types/plugin-input.ts src/types/env.ts src/types/permits.ts src/handlers/generate-payout-permit.ts src/handlers/index.ts src/handlers/generate-erc20-transfer.ts tests/generate-payout-permit.test.ts