@PabloCastellano
Great work on adding gas and balance checking to the topup and create-batch commands! The feature set is solid and will prevent many transaction failures.
However, I've identified a resource efficiency issue that should be addressed before merging:
Problem: Multiple Provider Instances
The current implementation creates 3 separate JsonRpcProvider instances per command execution:
- In
checkXdaiBalance() - line 51 of bzz-transaction-utils.ts
- In
calculateAndDisplayCosts() - line 87 of bzz-transaction-utils.ts
- In
checkAndApproveAllowance() - via makeReadySigner() at line 113
This causes:
- ❌ 3 RPC connections instead of 1
- ❌ Wasted network resources
- ❌ Potential connection issues under load
- ❌ Unused
provider object returned from calculateAndDisplayCosts()
Solution: Provider Caching
Implement a singleton pattern to reuse a single provider instance:
// Add to top of bzz-transaction-utils.ts
let cachedProvider: providers.JsonRpcProvider | null = null
function getProvider(): providers.JsonRpcProvider {
if (!cachedProvider) {
const jsonRpcUrl = process.env.SWARM_CLI_RPC_URL ?? 'https://xdai.fairdatasociety.org'
cachedProvider = new providers.JsonRpcProvider(jsonRpcUrl, getNetworkId())
}
return cachedProvider
}
Then update each function:
// Line 3: Change import
import { getNetworkId, Contracts, ABI } from './contracts'
// checkXdaiBalance() - line 67
const provider = getProvider() // Reuse cached provider
// calculateAndDisplayCosts() - line 90
const provider = getProvider() // Reuse cached provider
// Remove 'provider' from return type - it's unused by callers
// checkAndApproveAllowance() - line 112
const jsonRpcUrl = process.env.SWARM_CLI_RPC_URL ?? 'https://xdai.fairdatasociety.org'
Benefits
- ✅ Reduces provider instances from 3 to 1 per command
- ✅ Makes RPC URL configurable via
SWARM_CLI_RPC_URL env var
- ✅ Uses
getNetworkId() function to respect SWARM_CLI_NETWORK_ID override
- ✅ Better resource management and performance
Testing
After implementation:
npm test
npm run build
npm run cli -- stamp topup --amount 1000 --stamp <TEST_STAMP> --yes
This is a straightforward optimization that complements your solid feature work. Happy to help if you need the complete file or have questions!
Related Files:
src/utils/bzz-transaction-utils.ts - Provider caching implementation
src/command/stamp/topup.ts - Uses optimized functions (no changes needed)
src/command/utility/create-batch.ts - Uses optimized functions (no changes needed)
@PabloCastellano
Great work on adding gas and balance checking to the topup and create-batch commands! The feature set is solid and will prevent many transaction failures.
However, I've identified a resource efficiency issue that should be addressed before merging:
Problem: Multiple Provider Instances
The current implementation creates 3 separate
JsonRpcProviderinstances per command execution:checkXdaiBalance()- line 51 ofbzz-transaction-utils.tscalculateAndDisplayCosts()- line 87 ofbzz-transaction-utils.tscheckAndApproveAllowance()- viamakeReadySigner()at line 113This causes:
providerobject returned fromcalculateAndDisplayCosts()Solution: Provider Caching
Implement a singleton pattern to reuse a single provider instance:
Then update each function:
Benefits
SWARM_CLI_RPC_URLenv vargetNetworkId()function to respectSWARM_CLI_NETWORK_IDoverrideTesting
After implementation:
This is a straightforward optimization that complements your solid feature work. Happy to help if you need the complete file or have questions!
Related Files:
src/utils/bzz-transaction-utils.ts- Provider caching implementationsrc/command/stamp/topup.ts- Uses optimized functions (no changes needed)src/command/utility/create-batch.ts- Uses optimized functions (no changes needed)