feat(sdk): Add gasless deposit type exports and comprehensive test coverage#915
feat(sdk): Add gasless deposit type exports and comprehensive test coverage#915lrsaturnino wants to merge 16 commits intomainfrom
Conversation
…verage Implements SDK type exports and test infrastructure for gasless tBTC deposits feature, where relayer backend pays all gas fees for deposit reveals. Type System: - Export GaslessDepositResult interface for deposit initialization results - Export GaslessRevealPayload interface for backend relay submission - Add type export validation tests to prevent regressions Test Coverage: - Add comprehensive unit tests for initiateGaslessDeposit (L1 and L2 paths) - Add comprehensive unit tests for buildGaslessRelayPayload (owner encoding) - Verify error handling for unsupported chains and uninitialized contracts - Validate Bitcoin recovery address formats (P2PKH, P2WPKH) - Test chain-specific owner encoding (bytes32 for L1, address for L2) Documentation: - Add JSDoc documentation to gasless deposit helper methods - Document depositKey computation happens on backend (Bitcoin hash256) - Document chain-specific owner encoding requirements Code Quality: - Remove unused test variables - Fix all linting errors - Ensure all 683 tests pass with zero regressions This implementation ensures proper Bitcoin hash256 usage and chain-specific encoding for gasless deposit functionality.
piotr-roslaniec
left a comment
There was a problem hiding this comment.
Just some nitpicks/questions
|
|
||
| // Step 3: Determine owner encoding based on chain | ||
| // L1 contracts expect bytes32 owner (32 bytes), L2 contracts expect address (20 bytes) | ||
| let destinationOwner: string |
There was a problem hiding this comment.
Would ethers.utils.isHexString be useful here?
There was a problem hiding this comment.
yeah, that makes sense. We're doing manual validation and ethers already handles edge cases, so using it would be cleaner and less error-prone.
There was a problem hiding this comment.
Thank you for the suggestion! However, ethers.utils.isHexString would be redundant here since the Hex class constructor already validates hex format with regex, ensuring receipt.extraData.toPrefixedString() always returns valid hex strings.
| * Target chain name for the deposit. | ||
| * Can be "L1" or any L2 chain name (e.g., "Arbitrum", "Base", "Optimism"). | ||
| */ | ||
| destinationChainName: string |
There was a problem hiding this comment.
Consider type GaslessDestination = 'L1' | DestinationChainName
There was a problem hiding this comment.
I like that. It makes the intent clearer than just string and gives better autocomplete. Could we also add a runtime check or validation function to ensure it's actually 'L1' or a valid DestinationChainName? Otherwise TypeScript won't catch it at runtime if someone passes a wrong string.
There was a problem hiding this comment.
Good catch - implemented in #965336f2
…dd override setter in DepositsService
…d update error expectation
…-resolve + setter)
…normalization Add chain validation and backend-compatible chain name normalization for gasless tBTC deposits to improve error handling and ensure seamless backend integration. Changes: - Update testnet NativeBTCDepositor address to deployed Sepolia contract - Add SUPPORTED_GASLESS_CHAINS constant to validate deposit chains early - Implement chain name normalization in buildGaslessRelayPayload (SDK accepts capitalized names, backend receives lowercase) - Enhance JSDoc documentation with supported chains and capitalization - Add comprehensive test coverage for validation and normalization Technical details: - Supported chains: L1, Arbitrum, Base, Sui, StarkNet - Solana excluded due to different architecture - Chain names normalized to lowercase except "L1" - Early validation prevents runtime errors with clear messages Test results: 55 passing, 0 failing
…supported chains Update GaslessRevealPayload documentation to accurately reflect chain-specific parameter types based on deployed contract ABIs: - L1, Sui, StarkNet use bytes32 (32-byte hex) - Arbitrum, Base use address (20-byte hex) Add note about backend automatic padding for chains requiring bytes32 format.
Introduce GaslessDestination type to provide compile-time type safety for destination chain names in gasless deposit operations. Changes: - Add GaslessDestination type: 'L1' | DestinationChainName - Update GaslessDepositResult.destinationChainName from string to GaslessDestination - Update test imports and type annotations for type safety This prevents invalid chain names at compile time while maintaining runtime validation for unsupported chains. Type resolves to: 'L1' | 'Arbitrum' | 'Base' | 'Solana' | 'StarkNet' | 'Sui'
Updates CI workflows and package requirements to use Node.js 20.x to resolve dependency incompatibility with @solana/codecs-numbers@2.3.0 which requires Node.js >=20.18.0. Changes: - Update GitHub Actions workflows to use Node.js 20.x - Update package.json engines field to require Node.js >=20 - Resolves installation failures in CI/CD pipeline All TypeScript SDK tests verified passing with Node 20.19.0.
| const receipt = await this.generateDepositReceipt( | ||
| bitcoinRecoveryAddress, | ||
| depositor, | ||
| undefined |
There was a problem hiding this comment.
Not sure how to address this, but maybe there is a way to avoid passing undefined here and resolve this field earlier instead of using receipt.extraData, see line 591
There was a problem hiding this comment.
thanks for the review - fixed.
| if ( | ||
| !BitcoinScriptUtils.isP2PKHScript(recoveryOutputScript) && | ||
| !BitcoinScriptUtils.isP2WPKHScript(recoveryOutputScript) | ||
| ) { | ||
| throw new Error("Bitcoin recovery address must be P2PKH or P2WPKH") | ||
| } |
There was a problem hiding this comment.
We have some similar logic on line 738, maybe we should validate once (early) or deduplicate this logic into a utility if validating once is not practical
piotr-roslaniec
left a comment
There was a problem hiding this comment.
Just some comments/nitpicks
Fixed critical bug where L1 gasless deposits would send tBTC to the NativeBTCDepositor contract instead of the user's address. Changes: - Add depositOwner parameter to initiateGaslessDeposit() method - Encode depositOwner as bytes32 in L1 deposits via extraData - Simplify buildGaslessRelayPayload() to always require extraData - Update all test cases to pass depositOwner parameter - Remove unused encodeOwnerAddressAsBytes32() helper method The depositOwner is now properly embedded in the deposit script's extraData field for both L1 and L2 deposits, ensuring tBTC is transferred to the correct recipient address.
Co-authored-by: piotr-roslaniec <39299780+piotr-roslaniec@users.noreply.github.com>
Remove redundant validation logic in initiateGaslessDeposit that was duplicating the validation already performed in generateDepositReceipt. Changes: - Remove duplicate Bitcoin recovery address validation (lines 415-426) - Fix buildGaslessRelayPayload to handle missing extraData for L1 deposits - Encode depositor address as bytes32 when extraData is absent for L1 - Maintain strict extraData requirement for L2 cross-chain deposits Benefits: - Eliminates duplicate RPC call to getNetwork() (~100-200ms savings) - Single source of truth for address validation in generateDepositReceipt - Consistent validation flow across all deposit methods - Better L1 backward compatibility with optional extraData All 693 tests passing, including 26 gasless deposit tests.
…network/tbtc-v2 into feat/gasless-deposits
Add type assertions to test cases that intentionally use invalid chain names for error handling validation. This fixes TypeScript compilation errors while preserving the runtime validation tests. Changes: - test/services/deposits.test.ts: Add 'as GaslessDestination' type assertions to 5 test cases that verify rejection of invalid chain names (Optimism, arbitrum, InvalidChain, empty string) - api-reference/classes/DepositsService.md: Update parameter type documentation from 'string' to 'GaslessDestination' The type assertions allow TypeScript to compile while the tests still properly validate runtime error handling for unsupported chain names.
This commit fixes a critical bug in the gasless deposit flow where all L2 chains were receiving 20-byte Ethereum addresses, which is incorrect for non-EVM chains like Sui and StarkNet that require 32-byte addresses. Changes: - Add EVM_L2_CHAINS constant to identify EVM-compatible L2s (Arbitrum, Base) - Add isEVML2Chain() helper method for chain type detection - Update buildGaslessRelayPayload() to use chain-specific owner extraction: - EVM L2s (Arbitrum, Base): Extract 20-byte address from 32-byte extraData - Non-EVM L2s (Sui, StarkNet): Use full 32-byte extraData as owner - Fix existing tests to use correct 32-byte fixtures for non-EVM chains - Add comprehensive test coverage for all chain types and edge cases - Improve JSDoc documentation for gasless deposit methods This ensures deposits to Sui and StarkNet chains will use the correct address format, preventing deposit failures on non-EVM L2 chains.
Summary
Implements SDK type exports and test infrastructure for gasless tBTC deposits feature, where relayer backend pays all gas fees for deposit reveals.
This PR establishes the foundation for gasless deposits by adding type exports, comprehensive test coverage, and documentation.
Changes
Type System
Test Coverage
Documentation
Code Quality
Technical Details
Owner Encoding:
Bitcoin Algorithm:
Files Changed
Checklist