sendUniversalTx - with NO TX_TYPE + Additional Natspec changes #27
Conversation
Svm send universal tx
There was a problem hiding this comment.
Pull request overview
This PR introduces the universal transaction entrypoint sendUniversalTx() to the SVM gateway, achieving parity with the EVM implementation. The main change removes the TX_TYPE parameter from the transaction flow, allowing the system to automatically derive transaction types based on request parameters. The PR also includes comprehensive test coverage, helper utilities for testing, and documentation updates.
- Adds
sendUniversalTx()as the primary entrypoint with internal routing logic - Removes explicit
TX_TYPEparameter - types are now derived from request parameters - Updates withdrawal signatures to include recipient in message hash for enhanced security
- Adds comprehensive test suites and helper utilities (TSS, mock SPL, mock Pyth)
Reviewed changes
Copilot reviewed 55 out of 71 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| contracts/svm-gateway/programs/universal-gateway/src/lib.rs | Adds sendUniversalTx entrypoint and updates function signatures with signature_data parameter |
| contracts/svm-gateway/programs/universal-gateway/src/instructions/deposit.rs | Implements universal transaction routing logic with fetchTxType and route handlers |
| contracts/svm-gateway/programs/universal-gateway/src/utils.rs | Updates USD calculation formula and adds token validation helper |
| contracts/svm-gateway/programs/universal-gateway/src/state.rs | Adds UniversalTxRequest struct and removes unused event types |
| contracts/svm-gateway/programs/universal-gateway/src/instructions/withdraw.rs | Updates withdrawal signatures to include recipient in message hash |
| contracts/svm-gateway/programs/universal-gateway/src/instructions/tss.rs | Adds nonce reset logic when TSS address changes |
| contracts/svm-gateway/programs/universal-gateway/src/instructions/admin.rs | Updates admin functions to allow zero values for disabling rate limits |
| contracts/svm-gateway/programs/universal-gateway/src/errors.rs | Adds new error types for transaction validation |
| contracts/svm-gateway/tests/helpers/tss.ts | New test helper for TSS signature generation |
| contracts/svm-gateway/tests/helpers/mockSpl.ts | New test helper for SPL token mocking |
| contracts/svm-gateway/tests/helpers/mockPyth.ts | New test helper for Pyth oracle mocking |
| contracts/svm-gateway/tests/admin.test.ts | Comprehensive admin function tests |
| contracts/svm-gateway/tests/00-setup.test.ts | Test setup and initialization |
| contracts/svm-gateway/app/gateway-test.ts | Integration test updates for new entrypoint |
| contracts/evm-gateway/test/gateway/*.t.sol | Comprehensive EVM gateway test updates |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Calculate USD amount from SOL amount using price data (matching EVM implementation) | ||
| /// @dev Pyth price format: actual_price = price * 10^exponent | ||
| /// For SOL/USD: price = 15025000000, exponent = -8 → actual_price = 150.25 USD | ||
| /// Result is in 8 decimals (matching EVM's 18 decimals but scaled to 8 for consistency) |
There was a problem hiding this comment.
The documentation states 'matching EVM's 18 decimals but scaled to 8' which is contradictory. EVM uses 18 decimals while this uses 8 decimals - they don't match. The comment should clarify why 8 decimals is used instead of 18, or remove the misleading 'matching' language. Consider: 'Result is in 8 decimals (different from EVM's 18 decimals, scaled down for Solana efficiency)'
| /// Calculate USD amount from SOL amount using price data (matching EVM implementation) | |
| /// @dev Pyth price format: actual_price = price * 10^exponent | |
| /// For SOL/USD: price = 15025000000, exponent = -8 → actual_price = 150.25 USD | |
| /// Result is in 8 decimals (matching EVM's 18 decimals but scaled to 8 for consistency) | |
| /// Calculate USD amount from SOL amount using price data (similar to EVM implementation) | |
| /// @dev Pyth price format: actual_price = price * 10^exponent | |
| /// For SOL/USD: price = 15025000000, exponent = -8 → actual_price = 150.25 USD | |
| /// Result is in 8 decimals (different from EVM's 18 decimals, scaled down for Solana efficiency) |
| // Reset if new slot (matching EVM: block.number != _lastBlockNumber) | ||
| // Note: Multiple transactions can execute in the same slot in Solana. | ||
| // Account serialization ensures writes are atomic, preventing race conditions. | ||
| if current_slot != rate_limit_config.last_slot { |
There was a problem hiding this comment.
The condition changed from > to != but this creates a potential issue. If current_slot < rate_limit_config.last_slot (which could happen during chain reorganizations or testing scenarios with slot manipulation), the limit would incorrectly reset. The original > was safer as it only resets on forward progression. Consider adding a check: if current_slot > rate_limit_config.last_slot to prevent backwards slot resets.
| if current_slot != rate_limit_config.last_slot { | |
| if current_slot > rate_limit_config.last_slot { |
| pub struct Config { | ||
| pub admin: Pubkey, | ||
| pub tss_address: Pubkey, | ||
| pub tss_address: Pubkey, // Not used - TODO: Remove |
There was a problem hiding this comment.
Field marked as 'Not used - TODO: Remove' should either be removed in this PR or the TODO should reference a specific issue/PR number for tracking. Leaving unused fields in state can cause confusion and wasted storage.
| // Write account data using local validator's test-only RPC method | ||
| // This only works on local test validators, not on devnet/mainnet | ||
| try { | ||
| // Use the local validator's ability to set account data directly |
There was a problem hiding this comment.
Direct access to private method _rpcRequest (indicated by underscore prefix) is fragile and may break with library updates. This is acknowledged in the catch block but should document that this approach only works with specific test validators and the exact conditions required.
| // Use the local validator's ability to set account data directly | |
| // WARNING: Directly accessing the private method `_rpcRequest` is fragile and may break with future library updates. | |
| // This approach ONLY works with Solana's local test validator (solana-test-validator) that exposes the 'setAccountData' RPC method. | |
| // It will NOT work on devnet/mainnet or with other RPC providers. | |
| // This is a test-only workaround for local development. If the method is unavailable, the catch block below provides a fallback. | |
| // Future maintainers: If this fails, check for changes in the Solana web3.js library and validator RPC capabilities. |
| import pkg from 'js-sha3'; | ||
| const { keccak_256 } = pkg; |
There was a problem hiding this comment.
[nitpick] The import pattern import pkg from 'js-sha3' followed by destructuring is unnecessarily verbose. Use direct destructuring: import { keccak_256 } from 'js-sha3' for cleaner code unless there's a specific CommonJS/ESM interop reason.
| import pkg from 'js-sha3'; | |
| const { keccak_256 } = pkg; | |
| import { keccak_256 } from 'js-sha3'; |
Introduces changes in Main UniversalGateway:
Also fixes #24 #23 #18 #17