*Tx functions for all write fn in contracts#2085
Conversation
alexcos20
requested review from
andreip136,
bogdanfazakas,
giurgiur99,
mihaisc and
ndrpp
as code owners
|
/run-security-scan |
alexcos20
left a comment
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: medium
Summary:
This PR excellently introduces a decoupled transaction preparation architecture by adding *Tx methods that return TransactionRequest. This is a fantastic step for enabling multisig and offline signing flows via .populateTransaction(). However, the refactoring introduces a pervasive performance regression where estimateGas is redundantly executed twice per method call. There are also instances of missing await keywords that will lead to unhandled Promise rejections, and object mutations that may produce unexpected side-effects.
Comments:
• [ERROR][performance] Double Gas Estimation: Throughout this PR, most parent methods (e.g., buyDatatokenBatch, addNFTTemplate, dispense, etc.) unconditionally execute .estimateGas before passing control to their respective *Tx method, which also executes .estimateGas internally. This results in two RPC calls per transaction, significantly degrading SDK performance.
Please wrap the initial gas estimation in an if (estimateGas) check, similar to the correct implementation you wrote in AccessList.ts. This pattern must be applied globally across all refactored files.
Example fix:
• [ERROR][bug] this.amountToUnits is an asynchronous function and must be awaited. Passing unresolved Promises into .estimateGas will throw an error in Ethers.js. Combine the missing await fixes with the if (estimateGas) pattern discussed above to resolve both issues.
• [WARNING][bug] Avoid mutating the caller's dispenserParams object by modifying properties directly (e.g. dispenserParams.maxBalance = ...). This mutates the original object passed by reference, leading to unexpected side effects in the consumer application if the object is reused. Instead, create a formatted copy:
• [ERROR][bug] In the parent function createNftWithDatatokenWithDispenser, .estimateGas is currently being called with the unformatted dispenserParams (where values haven't been converted to wei). This will result in inaccurate gas estimations or contract reverts for token amounts. Fix this while also applying the if (estimateGas) optimization pattern:
• [INFO][bug] Good catch fixing the tokens array length mismatch issue for the .estimateGas call. Passing the correct tokensWithSufficientFunds prevents potential EVM reverts.
• [INFO][style] Excellent encapsulation of transaction preparation logic. Leveraging .populateTransaction() keeps internal Ethers.js API usage clean and standardized for building unsigned transactions. Good job!
|
/run-security-scan |
alexcos20
left a comment
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: low
Summary:
The PR introduces an excellent architectural upgrade by splitting execution methods into unsigned transaction builders (*Tx methods) across the SDK's smart contracts. This improves modularity and unlocks capabilities like offline signing and Safe (multi-sig) integration. The implementation is robust overall, though there are a few minor logic bugs (such as name/symbol overwrites), an inherited issue in the Escrow contract involving empty arrays, and some textual typos to clean up.
Comments:
• [WARNING][bug] When destructuring generateDtName(), if the user provides a valid name but omits the symbol (or vice versa), the provided value will be overwritten by the randomly generated one. Consider assigning them independently to preserve valid user inputs.
- if (!name || !symbol) {
- ;({ name, symbol } = generateDtName())
- }
+ if (!name || !symbol) {
+ const generated = generateDtName()
+ name = name || generated.name
+ symbol = symbol || generated.symbol
+ }• [ERROR][bug] tokensWithSufficientFunds is initialized as an empty array and never populated in prepareWithdrawInputs. When passed back and used in withdrawTx for gas estimation and transaction building, it will likely cause length-mismatch reverts in the EVM since amountsParsed contains values. This appears to be a legacy bug, but the new refactor makes it more prominent since it's now used in estimateGas. Consider mapping the original tokens array or fully implementing the filtering logic.
- return { tokensWithSufficientFunds, amountsWithSufficientFunds, amountsParsed }
+ return { tokensWithSufficientFunds: tokens, amountsWithSufficientFunds: amounts, amountsParsed }• [INFO][style] Since getEventFromTx in ContractUtils.ts has been upgraded to gracefully parse logs across EVENT_INTERFACES (which now includes AccessListFactory.abi), this manual log parsing block is redundant. You can simplify this by utilizing the shared utility.
- const parsedEvent = trxReceipt.logs
- .map((log) => {
- try {
- return this.contract.interface.parseLog(log)
- } catch {
- return null
- }
- })
- .find((event) => event?.name === 'NewAccessList')
- return parsedEvent?.args?.[0]
+ const parsedEvent = getEventFromTx(trxReceipt, 'NewAccessList')
+ return parsedEvent?.args?.[0]• [INFO][style] Excellent use of the spread operator here to construct dispenserParamsInUnits. Creating a new object instead of mutating the user-provided dispenserParams avoids unexpected side effects for consumers of the SDK. Good adherence to immutability best practices.
• [INFO][architecture] The fallback loop introduced here that iterates through EVENT_INTERFACES to parse raw logs is a great architectural addition. It elegantly solves the issue of missing parsed events when manually passing TransactionRequest objects via sendTransaction.
• [WARNING][style] There is an introduced typo here: config.transactio nPollingTimeout instead of config.transactionPollingTimeout. Even though this block is currently wrapped in a multi-line comment /* ... */, it's best to fix it to prevent a syntax error if the code is uncommented in the future.
- contract.transactionPollingTimeout = config.transactio nPollingTimeout
+ contract.transactionPollingTimeout = config.transactionPollingTimeout• [INFO][style] Several unexpected spaces were introduced into the JSDoc comments in this file, likely due to an accidental search/replace. For example:
- Line 58:
of the fair gas price - Line 74:
decimal p laces - Line 87:
conver ted amount - Line 110:
conve rted amount
Consider cleaning these up to maintain professional documentation quality.
|
/run-security-scan |
alexcos20
left a comment
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: low
Summary:
The PR successfully abstracts transaction creation logic by introducing xxxTx methods that generate unsigned TransactionRequests. This gives developers much greater flexibility to inspect, sign, or delegate transactions before executing them. Several bugs involving asynchronous amountToUnits calls have been properly awaited, and excellent fail-fast client-side validation logic has been added. Overall, the code quality is high, with just a few areas where input object mutation could cause side effects.
Comments:
• [WARNING][bug] Since you are refactoring this logic, it is a great opportunity to fix a pre-existing issue. Mutating the nftData object directly is an anti-pattern and can cause unexpected side effects for callers if they rely on or reuse the object. Consider creating a shallow copy.
public async createNFTTx(
nftData: NftCreateData,
estimatedGas?: bigint
): Promise<TransactionRequest> {
- if (!nftData.templateIndex) nftData.templateIndex = 1
- if (!nftData.name || !nftData.symbol) {
- const { name, symbol } = generateDtName()
- nftData.name = name
- nftData.symbol = symbol
- }
- if (nftData.templateIndex > (await this.getCurrentNFTTemplateCount())) {
+ const normalizedData = { ...nftData }
+ if (!normalizedData.templateIndex) normalizedData.templateIndex = 1
+ if (!normalizedData.name || !normalizedData.symbol) {
+ const { name, symbol } = generateDtName()
+ normalizedData.name = name
+ normalizedData.symbol = symbol
+ }
+ if (normalizedData.templateIndex > (await this.getCurrentNFTTemplateCount())) {Make sure to replace all subsequent usages of nftData with normalizedData in this function.
• [WARNING][bug] Similar to NFTFactory, modifying fixedRateParams directly can cause side effects. Use local variables instead, just like you beautifully handled safeConsumeMarketFee in startOrderTx!
- if (!fixedRateParams.allowedConsumer) fixedRateParams.allowedConsumer = ZERO_ADDRESS
- const withMint = fixedRateParams.withMint === false ? 0 : 1
+ const safeAllowedConsumer = fixedRateParams.allowedConsumer || ZERO_ADDRESS
+ const safeWithMint = fixedRateParams.withMint === false ? 0 : 1
const addressArgs = [
fixedRateParams.baseTokenAddress,
fixedRateParams.owner,
fixedRateParams.marketFeeCollector,
- fixedRateParams.allowedConsumer
+ safeAllowedConsumer
]
const uintArgs = [
fixedRateParams.baseTokenDecimals,
fixedRateParams.datatokenDecimals,
fixedRateParams.fixedRate,
fixedRateParams.marketFee,
- withMint
+ safeWithMint
]• [WARNING][bug] Avoid mutating the dispenserParams object directly.
- if (!dispenserParams.allowedSwapper) dispenserParams.allowedSwapper = ZERO_ADDRESS
- dispenserParams.withMint = dispenserParams.withMint !== false
+ const safeAllowedSwapper = dispenserParams.allowedSwapper || ZERO_ADDRESS
+ const safeWithMint = dispenserParams.withMint !== false
const estGas =
estimatedGas ??
(await dtContract.createDispenser.estimateGas(
dispenserAddress,
dispenserParams.maxTokens,
dispenserParams.maxBalance,
- dispenserParams.withMint,
- dispenserParams.allowedSwapper
+ safeWithMint,
+ safeAllowedSwapper
))Don't forget to also update the variables passed in the buildUnsignedTx call at the bottom of the method.
• [INFO][style] The signer parameter is not used anywhere in the body of sendPreparedTx. You can safely remove it to clean up the signature.
export async function sendPreparedTx(
- signer: Signer,
functionToSend: BaseContractMethod,
args: any[],
overrides: Record<string, any>
): Promise<TransactionResponse> {(If removed, ensure you update the call inside sendTx to return sendPreparedTx(functionToSend, args, overrides)).
• [INFO][other] This is a great change! Previously, invalid withdrawals were silently skipped. Throwing an error provides a much better fail-fast behavior and prevents silent partial failures. Excellent catch.
• [INFO][other] The manual fallback mechanism for iterating over EVENT_INTERFACES when eventSignature is not exposed by ethers is a very robust and well-thought-out solution. Good job!
|
/run-security-scan |
alexcos20
left a comment
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: high
Summary:
This PR introduces a significant refactor to decouple transaction preparation (buildUnsignedTx) from execution, which is an excellent architectural improvement for supporting offline signing, multisig flows, and intent-based transactions. It also brings some nice fixes like addressing a floating promise in EnterpriseFeeCollector and improving error handling in Escrow.withdraw. However, there are multiple critical bugs: several methods bypass the estimateGas check, resulting in unintended transaction execution on the blockchain instead of returning the gas estimate. Furthermore, an important validation check was removed in Escrow.authorize.
Comments:
• [ERROR][bug] The estimateGas check is missing here. Calling this method with estimateGas = true will unexpectedly execute the transaction instead of just returning the gas limit, costing users actual gas and breaking the API contract.
public async approve<G extends boolean = false>(
dtAddress: string,
spender: string,
amount: string,
estimateGas?: G
): Promise<ReceiptOrEstimate<G>> {
const tx = await this.approveTx(dtAddress, spender, amount)
+ if (estimateGas) return <ReceiptOrEstimate<G>>tx.gasLimit
const trxReceipt = await sendPreparedTransaction(this.getSignerAccordingSdk(), tx)
return <ReceiptOrEstimate<G>>trxReceipt
}Note: This is a systematic omission across the PR. Please also add this check in:
AccessList.mintDatatoken.buyFromDispenserAndOrderDispenser.createEscrow.depositEscrow.authorizeFixedRateExchange.buyDatatokensFixedRateExchange.sellDatatokensNFT.setTokenURINFTFactory.createNftWithDatatokenWithFixedRateNFTFactory.createNftWithDatatokenWithDispenser
• [ERROR][bug] Theauthorizemethod previously checkedthis.getAuthorizationsand immediately returnednullif the payee was already authorized, preventing redundant gas expenditure. This logic was completely removed. Please reinstate the check either here or insideauthorizeTx.
+ const auths = await this.getAuthorizations(
+ token,
+ await this.signer.getAddress(),
+ payee
+ )
+ if (auths.length !== 0) {
+ console.log(`Payee ${payee} already authorized`)
+ return null
+ }
const tx = await this.authorizeTx(• [ERROR][bug] This snippet was accidentally pasted inside the JSDoc comment block, breaking the documentation while providing no functionality. Since getTokenURI is a read-only method, this line should simply be removed.
* @param {string} nftAddress - The address of the NFT.
* @param {number} id - The ID of the token.
* @returns {Promise<string>}
- if (estimateGas) return <ReceiptOrEstimate<G>>tx.gasLimit
*/• [WARNING][style] You are manually mapping and parsing the logs here. Since AccessListFactory.abi was added to EVENT_INTERFACES in ContractUtils.ts as part of this PR, you can simply reuse getEventFromTx for consistency and cleaner code.
- const parsedEvent = trxReceipt.logs
- .map((log) => {
- try {
- return this.contract.interface.parseLog(log)
- } catch {
- return null
- }
- })
- .find((event) => event?.name === 'NewAccessList')
- return parsedEvent?.args?.[0]
+ const event = getEventFromTx(trxReceipt, 'NewAccessList')
+ return event?.args?.[0]• [INFO][bug] Great catch! Adding await to amountToUnits fixes what was previously a floating promise bug. Excellent attention to detail here.
• [INFO][other] Failing fast by throwing an error upfront instead of silently ignoring invalid withdrawal amounts is a great DX improvement to avoid unexpected partial withdrawals.
|
/run-security-scan |
alexcos20
left a comment
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: high
Summary:
This PR successfully refactors the contract interaction patterns by decoupling transaction request generation (*Tx methods) from transaction dispatching. This is a highly beneficial architectural change for accommodating varied signing strategies. However, there are a few critical regressions related to missing ethers imports and unhandled early returns that break backward compatibility and will cause runtime errors.
Comments:
• [ERROR][bug] The ethers namespace is not explicitly imported, so referencing ethers.Interface and ethers.id will cause a runtime ReferenceError: ethers is not defined. Please import Interface and id directly from ethers.
-import {
- Signer,
- Contract,
- TransactionResponse,
- TransactionRequest,
- BaseContractMethod,
- formatUnits,
- parseUnits,
- EventLog,
- TransactionReceipt
-} from 'ethers'
+import {
+ Signer,
+ Contract,
+ TransactionResponse,
+ TransactionRequest,
+ BaseContractMethod,
+ formatUnits,
+ parseUnits,
+ EventLog,
+ TransactionReceipt,
+ Interface,
+ id
+} from 'ethers'And update the usages below:
- new ethers.Interface(ERC20Template.abi),
+ new Interface(ERC20Template.abi),
...
- return topic0 === ethers.id(log.eventSignature).toLowerCase()
+ return topic0 === id(log.eventSignature).toLowerCase()• [WARNING][bug] The early return check that verifies if a payee is already authorized was removed. This means the SDK will now broadcast an unnecessary transaction (which might fail or waste gas) instead of safely returning null. Please restore the early return check in authorizeTx and handle it in the authorize wrapper.
public async authorize<G extends boolean = false>(
...
): Promise<ReceiptOrEstimate<G>> {
const tx = await this.authorizeTx(
...
)
+ if (!tx) return null
if (estimateGas) return <ReceiptOrEstimate<G>>tx.gasLimit
const trxReceipt = await sendPreparedTransaction(this.getSignerAccordingSdk(), tx)
return <ReceiptOrEstimate<G>>trxReceipt
}
public async authorizeTx(
...
- ): Promise<TransactionRequest> {
+ ): Promise<TransactionRequest | null> {
+ const auths = await this.getAuthorizations(
+ token,
+ await this.signer.getAddress(),
+ payee
+ )
+ if (auths.length !== 0) {
+ console.log(`Payee ${payee} already authorized`)
+ return null
+ }• [WARNING][bug] The wrapper methods collectBasetokens, collectDatatokens, and collectOceanFee lost their early return checks (if (!exchange) return null). Calling them with an invalid exchangeId will now throw an error (from inside the *Tx methods) rather than returning null as they previously did. This breaks API behavior compatibility. Please restore the early checks in the wrappers.
public async collectBasetokens<G extends boolean = false>(
exchangeId: string,
amount: string,
estimateGas?: G
): Promise<ReceiptOrEstimate<G>> {
+ const exchange = await this.getExchange(exchangeId)
+ if (!exchange) return null
const tx = await this.collectBasetokensTx(exchangeId, amount)• [INFO][other] This is a great improvement to fail fast instead of silently filtering invalid entries, though note that it introduces a behavior change. Downstream users who relied on the previous silent-filtering behavior will need to sanitize their inputs.
• [INFO][style] Since AccessListFactory.abi was added to EVENT_INTERFACES inside ContractUtils.ts, you could simplify this log parsing loop by reusing the updated getEventFromTx.
- const parsedEvent = trxReceipt.logs
- .map((log) => {
- try {
- return this.contract.interface.parseLog(log)
- } catch {
- return null
- }
- })
- .find((event) => event?.name === 'NewAccessList')
- return parsedEvent?.args?.[0]
+ const event = getEventFromTx(trxReceipt, 'NewAccessList')
+ return event?.args?.[0]• [INFO][other] By setting overrides.gasLimit with the 20,000 unit buffer within buildTxOverrides, any SDK user calling wrapper methods with estimateGas: true will now receive the buffered gas limit (estGas + 20000n) rather than the raw estimated gas. This is generally safer for users constructing their own transactions, but represents a slight shift in what estimateGas literally returns.
|
gemini still hallucinates on some things, but PR is ready for review |
2 participants
Summary
This PR refactors contract write flows to support offline signing while preserving existing high-level APIs.
src/utils/ContractUtils.ts:buildTxOverrides(estGas, signer, gasFeeMultiplier)buildUnsignedTx(functionToSend, args, overrides)sendPreparedTx(signer, functionToSend, args, overrides)sendPreparedTransaction(signer, tx)sendTx(...)behavior intact, but internally reuse shared override/send helpers.*Txmethods across contract wrappers to return unsignedTransactionRequest.*Txmethod to build tx,sendPreparedTransaction(...).Why
Today, write methods both build and send transactions inline, which duplicates logic and makes offline signing difficult.
This change:
Scope
Core utility changes
src/utils/ContractUtils.tssendTx,Contract wrappers updated
src/contracts/AccessList.tssrc/contracts/AccessListFactory.tssrc/contracts/Datatoken.tssrc/contracts/Datatoken4.tssrc/contracts/Dispenser.tssrc/contracts/Escrow.tssrc/contracts/FixedRateExchange.tssrc/contracts/NFT.tssrc/contracts/NFTFactory.tssrc/contracts/Router.tsNew
*TxAPI surface (high level)AccessList: mint/batchMint/burn/ownership write ops now have*Tx.AccessListFactory: deploy + template-change operations now have*Tx.Datatoken+Datatoken4: major write operations now expose unsigned tx builders.Dispenser: create/activate/deactivate/setAllowedSwapper/dispense/ownerWithdraw*Tx.Escrow: deposit/withdraw/authorize/cancelExpiredLocks*Tx.FixedRateExchange: all core exchange write ops now expose*Tx.NFT: ERC20 creation, permission management, transfer, metadata, token URI, store writes now expose*Tx.NFTFactory: NFT/template and composite create/order flows now expose*Tx.Router: batch buy + approved-token/fixed-rate/dispenser/opc updates expose*Tx.Backward compatibility
estimateGaspaths are retained.Tests update
Cause: bignumber.js switches to exponential notation in .toString() when the magnitude is large enough (default EXPONENTIAL_AT is 21). Values around 1e21 wei hit that, so you got '1.070373e+21' while amountToUnits(...) still returns a full decimal string '1070373000000000000000'. Same number, different string → assertion fails.
Change: For those wei comparisons, use .toFixed(0) instead of .toString() on the BigNumber result, and normalize the event arg with .toString() when building the BigNumber (works cleanly with ethers bigint args).