Skip to content

feat: get transaction status #499

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jaiminRaiyani
Copy link
Contributor

@jaiminRaiyani jaiminRaiyani commented Apr 5, 2025

Add starknet_getTransactionStatus Implementation

Description

This PR adds support for the starknet_getTransactionStatus RPC method, allowing users to query the status of transactions on the Starknet network. The implementation includes:

  • A new TransactionStatus model with support for:
    • finality_status (RECEIVED, REJECTED, ACCEPTED_ON_L2, ACCEPTED_ON_L1)
    • execution_status (SUCCEEDED, REVERTED)
  • Comprehensive test coverage including:
    • Handling non-existent transactions
    • Processing real transactions
    • Validating transaction status responses
    • Testing both successful and failed scenarios

Testing

  • ✅ All unit tests passing
  • ✅ Integration tests with devnet passing
  • ✅ Tested with both V1 and V3 transaction formats
  • ✅ Verified proper error handling

Screenshot 2025-04-05 234618

Implementation Details

  • Added get_transaction_status.dart model with Freezed annotations
  • Implemented proper JSON serialization/deserialization
  • Added export in index.dart for public access
  • Included detailed test cases in get_transaction_status_test.dart

Related Issues

Closes #[#468] - Add support for starknet_getTransactionStatus

Checklist

  • Added new model with proper documentation
  • Added comprehensive test coverage
  • Updated exports in index.dart
  • Tested with devnet
  • Follows project coding standards

Summary by CodeRabbit

  • New Features

    • Introduced a new transaction status feature that provides detailed finality and execution information for improved monitoring of transactions.
    • Added documentation for the starknet_getTransactionStatus method, including usage examples and expected outcomes.
  • Tests

    • Added comprehensive tests to ensure the accuracy and reliability of transaction status queries and error handling.
  • Chores

    • Updated timeout configurations for testing to enhance test execution management.

Copy link

docs-page bot commented Apr 5, 2025

To view this pull requests documentation preview, visit the following URL:

docs.page/focustree/starknet.dart~499

Documentation is deployed and generated using docs.page.

Copy link
Contributor

coderabbitai bot commented Apr 5, 2025

Walkthrough

This pull request introduces a new TransactionStatus data model using the Freezed package. It adds the core model implementation along with generated files for JSON serialization and deserialization. The update also includes an export in the module’s index and a set of tests that validate the transaction status retrieval via RPC and proper JSON parsing.

Changes

File(s) Change Summary
packages/starknet_provider/.../get_transaction_status.dart
packages/starknet_provider/.../get_transaction_status.freezed.dart
packages/starknet_provider/.../get_transaction_status.g.dart
Adds the TransactionStatus model and generated code supporting immutability, JSON serialization, and deserialization using Freezed.
packages/starknet_provider/.../index.dart Exports the new get_transaction_status.dart module.
packages/starknet_provider/.../get_transaction_status_test.dart Introduces tests for validating transaction status retrieval, error handling, and JSON response parsing.
docs/packages/starknet-provider.mdx Adds documentation for the starknet_getTransactionStatus method, including usage examples and method description.
packages/starknet_provider/.../README.md Updates the implementation status of starknet_getTransactionStatus from not implemented to implemented.
packages/starknet_provider/.../dart_test.yaml Introduces new timeout configurations for testing tags, adding an integration tag and updating the unit tag.
packages/starknet_provider/.../read_provider.dart Adds getTransactionStatus method to ReadProvider and its implementation in JsonRpcReadProvider for retrieving transaction status.

Suggested labels

package:starknet_provider

Suggested reviewers

  • ptisserand

Poem

I'm a rabbit with a skip in my code trail,
Hopping through models where data sets sail.
TransactionStatus blossoms like a bright spring flower,
With JSON and Freezed bringing magic power.
Tests confirm each hop in this techy glade—
A joyful leap through improvements well-made! 🐰


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dbcbd77 and 1da5900.

📒 Files selected for processing (3)
  • packages/starknet_provider/dart_test.yaml (1 hunks)
  • packages/starknet_provider/lib/src/read_provider.dart (2 hunks)
  • packages/starknet_provider/test/get_transaction_status_test.dart (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/starknet_provider/test/get_transaction_status_test.dart
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: analyze
  • GitHub Check: test-integration
🔇 Additional comments (4)
packages/starknet_provider/lib/src/read_provider.dart (2)

138-142: Good, appropriate method declaration with proper documentation

The new getTransactionStatus method declaration is well-documented with a clear Dartdoc comment that includes a link to the specification. The method signature correctly takes a transaction hash parameter and returns a Future<TransactionStatus>. This aligns perfectly with the PR objective of implementing the starknet_getTransactionStatus RPC method.


365-372: Implementation looks good and follows established patterns

The implementation of getTransactionStatus follows the same consistent pattern as other methods in the class: calling callRpcEndpoint with the appropriate parameters and then transforming the result using the appropriate model's fromJson method. This ensures consistency throughout the codebase.

packages/starknet_provider/dart_test.yaml (2)

2-3: Good timeout configuration for integration tests

Adding an increased timeout multiplier (5x) for integration tests is appropriate, especially when dealing with network calls to RPC endpoints. This helps prevent tests from failing due to network latency when testing the new transaction status functionality.


5-5: Explicit timeout for unit tests adds clarity

Making the timeout multiplier for unit tests explicit (1x) improves configuration clarity, even though this is likely the default value. This provides a good contrast with the integration test timeout and makes the configuration more self-documenting.


🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai plan to trigger planning for file edits and PR creation.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (5)
packages/starknet_provider/lib/src/model/index.dart (1)

37-37: Export order should follow alphabetical pattern

The new export for 'get_transaction_status.dart' has been added at the end of the file, but the existing exports appear to be in alphabetical order. For consistency, consider moving this line between 'get_transaction_receipt.dart' (line 24) and 'invoke_transaction.dart' (line 25).

24 export 'get_transaction_receipt.dart';
+export 'get_transaction_status.dart';
25 export 'invoke_transaction.dart';
packages/starknet_provider/lib/src/model/get_transaction_status.dart (1)

1-15: Implementation looks good but could benefit from additional documentation

The TransactionStatus model is correctly implemented with the required and optional fields. The design follows the Freezed pattern for immutable data models with JSON serialization support.

Consider adding documentation comments to explain:

  1. The purpose of this class
  2. The meaning of each status value (RECEIVED, REJECTED, ACCEPTED_ON_L2, ACCEPTED_ON_L1, SUCCEEDED, REVERTED)
  3. When executionStatus might be null
@freezed
+/// Represents the status of a Starknet transaction.
+/// 
+/// Contains information about both the finality status (indicating where in the
+/// lifecycle the transaction is) and execution status (if it succeeded or failed).
class TransactionStatus with _$TransactionStatus {
  const factory TransactionStatus({
+    /// The finality status of the transaction.
+    /// Possible values:
+    /// - RECEIVED: Transaction is received but not yet processed
+    /// - REJECTED: Transaction was rejected
+    /// - ACCEPTED_ON_L2: Transaction is accepted on L2
+    /// - ACCEPTED_ON_L1: Transaction is accepted on L1
    required String finalityStatus,
+    /// The execution status of the transaction.
+    /// Possible values:
+    /// - SUCCEEDED: Transaction executed successfully
+    /// - REVERTED: Transaction execution failed
+    /// May be null if the transaction hasn't been executed yet.
    String? executionStatus,
  }) = _TransactionStatus;
packages/starknet_provider/test/get_transaction_status_test.dart (2)

13-34: Consider more robust test setup

The test setup establishes a connection to a local devnet and verifies it responds. While this works, it might be worth handling potential connection issues more gracefully.

Consider adding timeout handling and a more descriptive failure message:

setUp(() async {
  provider =
      JsonRpcReadProvider(nodeUri: Uri.parse('http://127.0.0.1:5050'));

  // First verify the devnet is responding
-  final chainIdResponse = await http.post(
-    Uri.parse('http://127.0.0.1:5050'),
-    headers: {'Content-Type': 'application/json'},
-    body: jsonEncode({
-      'jsonrpc': '2.0',
-      'method': 'starknet_chainId',
-      'params': [],
-      'id': 1,
-    }),
-  );
-  print('Chain ID response: ${chainIdResponse.body}');
+  try {
+    final chainIdResponse = await http.post(
+      Uri.parse('http://127.0.0.1:5050'),
+      headers: {'Content-Type': 'application/json'},
+      body: jsonEncode({
+        'jsonrpc': '2.0',
+        'method': 'starknet_chainId',
+        'params': [],
+        'id': 1,
+      }),
+    ).timeout(Duration(seconds: 5));
+    print('Chain ID response: ${chainIdResponse.body}');
+    
+    final result = jsonDecode(chainIdResponse.body);
+    expect(result['error'], isNull, 
+      reason: 'Failed to connect to devnet: ${chainIdResponse.body}');
+  } catch (e) {
+    fail('Failed to connect to devnet: $e. Make sure the devnet is running at http://127.0.0.1:5050');
+  }

  // Use a known transaction hash for non-existent transaction test
  txHash = Felt.fromHexString(
      '0x03b2911796e0024f9e23d7337997538058eca267d5ddaa582d482cbe1fb64897');
  print('Using tx_hash: ${txHash.toHexString()}');
});

1-241: Consider adding test for error handling

The test suite covers successful scenarios and parsing variations, but could be enhanced with tests for error handling.

Add a test for handling malformed responses, such as:

test('should handle malformed responses', () async {
  // Test missing required field
  final missingRequiredField = {
    'jsonrpc': '2.0',
    'id': 1,
    'result': {
      // No finality_status field
      'execution_status': 'SUCCEEDED'
    }
  };

  // Expecting an exception due to missing required field
  expect(
    () => TransactionStatus.fromJson(
        missingRequiredField['result'] as Map<String, dynamic>),
    throwsA(isA<TypeError>()),
  );

  // Test invalid field values
  final invalidFieldValues = {
    'jsonrpc': '2.0',
    'id': 1,
    'result': {
      'finality_status': 'INVALID_STATUS',
      'execution_status': 'UNKNOWN'
    }
  };

  // Should parse but values would not match expected enum values
  final status = TransactionStatus.fromJson(
      invalidFieldValues['result'] as Map<String, dynamic>);
  
  expect(status.finalityStatus, equals('INVALID_STATUS'));
  expect(status.executionStatus, equals('UNKNOWN'));
  
  // Verify that the values don't match expected enums
  expect(
    ['RECEIVED', 'REJECTED', 'ACCEPTED_ON_L2', 'ACCEPTED_ON_L1']
        .contains(status.finalityStatus),
    isFalse,
  );
  
  expect(
    ['SUCCEEDED', 'REVERTED'].contains(status.executionStatus),
    isFalse,
  );
});
packages/starknet_provider/lib/src/model/get_transaction_status.freezed.dart (1)

167-186: LGTM: Abstract class declaration matching implementation

The abstract class definition properly declares the interface that the implementation must satisfy, with the same required/optional parameters.

Consider adding an enumeration or constants for the possible values of finalityStatus (RECEIVED, REJECTED, ACCEPTED_ON_L2, ACCEPTED_ON_L1) and executionStatus (SUCCEEDED, REVERTED) in the non-generated file to improve type safety and provide better intellisense support.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1879fd0 and ee5aeef.

📒 Files selected for processing (5)
  • packages/starknet_provider/lib/src/model/get_transaction_status.dart (1 hunks)
  • packages/starknet_provider/lib/src/model/get_transaction_status.freezed.dart (1 hunks)
  • packages/starknet_provider/lib/src/model/get_transaction_status.g.dart (1 hunks)
  • packages/starknet_provider/lib/src/model/index.dart (1 hunks)
  • packages/starknet_provider/test/get_transaction_status_test.dart (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: analyze
  • GitHub Check: test-integration
  • GitHub Check: lint
🔇 Additional comments (10)
packages/starknet_provider/lib/src/model/get_transaction_status.g.dart (1)

1-22: Generated code looks correct

The JSON serialization code correctly maps between the snake_case JSON fields (finality_status, execution_status) and the camelCase Dart properties (finalityStatus, executionStatus). This is consistent with standard JSON conventions.

packages/starknet_provider/test/get_transaction_status_test.dart (3)

36-55: Test handles error case appropriately

Good test for handling non-existent transaction hash, verifying both the error code and message.


179-218: Good unit test for successful transaction parsing

This test effectively validates that the TransactionStatus model can correctly parse a JSON response with both finality_status and execution_status fields.


220-239: Good unit test for handling optional fields

This test properly verifies that the TransactionStatus model can handle a response with only the required finality_status field, where execution_status is missing.

packages/starknet_provider/lib/src/model/get_transaction_status.freezed.dart (6)

1-5: LGTM: Generated file header with appropriate ignores

The file correctly starts with standard ignores for generated code, which prevents linting issues and coverage reports.


6-11: LGTM: File part declaration and generator header

This correctly identifies the file as part of the main get_transaction_status.dart file, indicating the model definition is in that file.


36-74: LGTM: CopyWith implementation

The generated copyWith implementation follows the standard Freezed pattern with proper null safety handling.


116-129: LGTM: TransactionStatus implementation with proper serialization annotations

The implementation correctly:

  • Uses JsonSerializable for JSON conversion
  • Requires finalityStatus
  • Makes executionStatus optional

This matches the expected behavior for transaction status queries in Starknet.


130-164: LGTM: Standard object method overrides

The implementation correctly overrides:

  • toString() for debugging
  • equals operator for comparing instances
  • hashCode for use in collections
  • toJson() for serialization

These are all properly implemented following best practices.


21-34:

✅ Verification successful

TransactionStatus model structure looks good

The model correctly defines:

  • finalityStatus as required string
  • executionStatus as optional string

This aligns with the PR objectives which mentioned these two status types for the Starknet transaction status RPC method.


🌐 Web query:

What are the standard transaction statuses in StarkNet?

💡 Result:

Starknet transactions progress through distinct statuses reflecting their lifecycle from submission to finalization. These statuses are categorized into finality statuses (indicating transaction acceptance on the network) and execution statuses (indicating the outcome of transaction processing).

Finality Statuses

  1. RECEIVED:
    The transaction is validated by the mempool and awaits processing by the Sequencer. This is analogous to Ethereum’s mempool state[1][4].

  2. ACCEPTED_ON_L2:
    The transaction is included in a Starknet block and applied to the L2 state. This indicates L2 finality, though reversibility is theoretically possible if Sequencers collude[1][3][4].

  3. ACCEPTED_ON_L1:
    The transaction achieves absolute finality after the block’s STARK proof is verified on Ethereum L1. This aligns with Ethereum’s consensus security[1][3][7].

  4. REJECTED:
    The transaction fails initial validation (e.g., invalid signature or insufficient fees) and is excluded from blocks[1][4].

Execution Statuses

  1. SUCCEEDED:
    The transaction executed successfully and modified the L2 state as intended[1][4][5].

  2. REVERTED:
    The transaction passed validation but failed during execution (e.g., a smart contract error). State changes are rolled back, but the transaction is recorded in the block[1][4][5].

  3. NOT_RECEIVED:
    The transaction was not acknowledged by the Sequencer, typically due to network issues[4].

Key Workflow Examples

  • A transaction may transition:
    RECEIVEDACCEPTED_ON_L2ACCEPTED_ON_L1 (if fully finalized).
  • A failed transaction might follow:
    RECEIVEDREVERTED (execution failure) or REJECTED (validation failure).

Developers can check these statuses using tools like starkli transaction_receipt or Starknet-py’s fetch_transaction_receipt[1][8]. The distinction between L2 and L1 finality is critical for applications requiring Ethereum-level security guarantees[3][7].

Citations:


TransactionStatus model structure is validated and aligns with Starknet standards.
Both the required finalityStatus and optional executionStatus fields correctly reflect the two distinct status categories used in Starknet (e.g., finality statuses like RECEIVED, ACCEPTED_ON_L2, ACCEPTED_ON_L1, REJECTED and execution statuses like SUCCEEDED, REVERTED, NOT_RECEIVED). This confirms that the implementation meets the PR objectives and mirrors the statuses described in the official documentation.

@jaiminRaiyani jaiminRaiyani changed the title Feature/get transaction status feat/get transaction status Apr 5, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
docs/packages/starknet-provider.mdx (1)

68-69: Cleanup of Redundant Documentation
The removal of the outdated description for starknet_getBlockWithReceipts in the "Call read-only method" section helps declutter the documentation. Please verify that any crucial details for users regarding this method are still documented elsewhere if needed. Additionally, note the static analysis hint about loose punctuation; if reintroducing similar content in the future, consider tightening the phrasing.

🧰 Tools
🪛 LanguageTool

[uncategorized] ~68-~68: Loose punctuation mark.
Context: ...ethod - starknet_getBlockWithReceipts: Retrieves block information with transa...

(UNLIKELY_OPENING_PUNCTUATION)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ee5aeef and 8613897.

📒 Files selected for processing (1)
  • docs/packages/starknet-provider.mdx (2 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/packages/starknet-provider.mdx

[uncategorized] ~68-~68: Loose punctuation mark.
Context: ...ethod - starknet_getBlockWithReceipts: Retrieves block information with transa...

(UNLIKELY_OPENING_PUNCTUATION)

⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: test-integration
  • GitHub Check: lint
  • GitHub Check: analyze
🔇 Additional comments (2)
docs/packages/starknet-provider.mdx (2)

34-34: New RPC Method Entry Added
The addition of the starknet_getTransactionStatus row in the Supported JSON RPC methods table is clear and accurately reflects the new feature. Please ensure that the table’s column alignment and formatting remain consistent with the rest of the table.


70-88: Comprehensive Documentation for starknet_getTransactionStatus
The new documentation section for starknet_getTransactionStatus is thorough and well-structured. It provides a concise method description, an example with a code snippet, and details about the TransactionStatus model’s properties. Ensure that the field names (finalityStatus and executionStatus) are consistent with the implementation.

@jaiminRaiyani jaiminRaiyani changed the title feat/get transaction status feat: get transaction status Apr 5, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
packages/starknet_provider/test/get_transaction_status_test.dart (3)

171-194: Redundant test cases can be consolidated.

Tests "should return transaction status with required finality_status field" and "should handle transaction status with only finality_status field" are nearly identical, both checking that the transaction has a finality status. Consider consolidating them or making their purposes more distinct.

Consolidate the duplicate test cases or clarify their distinct purposes:

    test('should return transaction status with required finality_status field',
        () async {
      expect(successfulTxHash, isNotNull,
          reason: 'Previous test must succeed to get a valid transaction hash');

      final response = await http.post(
        devnetUri,
        headers: {'Content-Type': 'application/json'},
        body: jsonEncode({
          'jsonrpc': '2.0',
          'method': 'starknet_getTransactionStatus',
          'params': [successfulTxHash],
          'id': 1,
        }),
      );

      final result = jsonDecode(response.body);
      expect(result['error'], isNull,
          reason: 'Failed to get transaction status');

      final status = TransactionStatus.fromJson(result['result']);
      expect(status.finalityStatus, isNotNull,
          reason: 'Transaction status should have a finality status');
+     // Also verify fields are of expected types
+     expect(status.finalityStatus.toString(), matches(RegExp(r'(RECEIVED|REJECTED|ACCEPTED_ON_L2|ACCEPTED_ON_L1)')),
+         reason: 'Finality status should be one of the expected values');
    }, tags: ['integration']);

-    test('should handle transaction status with only finality_status field',
-        () async {
-      expect(successfulTxHash, isNotNull,
-          reason: 'Previous test must succeed to get a valid transaction hash');
-
-      final response = await http.post(
-        devnetUri,
-        headers: {'Content-Type': 'application/json'},
-        body: jsonEncode({
-          'jsonrpc': '2.0',
-          'method': 'starknet_getTransactionStatus',
-          'params': [successfulTxHash],
-          'id': 1,
-        }),
-      );
-
-      final result = jsonDecode(response.body);
-      expect(result['error'], isNull,
-          reason: 'Failed to get transaction status');
-
-      final status = TransactionStatus.fromJson(result['result']);
-      expect(status.finalityStatus, isNotNull,
-          reason: 'Transaction status should have a finality status');
-    }, tags: ['integration']);

Or if there's a meaningful distinction between these tests, clarify it by modifying the second test:

    test('should handle transaction status with only finality_status field',
        () async {
      expect(successfulTxHash, isNotNull,
          reason: 'Previous test must succeed to get a valid transaction hash');

      final response = await http.post(
        devnetUri,
        headers: {'Content-Type': 'application/json'},
        body: jsonEncode({
          'jsonrpc': '2.0',
          'method': 'starknet_getTransactionStatus',
          'params': [successfulTxHash],
          'id': 1,
        }),
      );

      final result = jsonDecode(response.body);
      expect(result['error'], isNull,
          reason: 'Failed to get transaction status');

      final status = TransactionStatus.fromJson(result['result']);
      expect(status.finalityStatus, isNotNull,
          reason: 'Transaction status should have a finality status');
+     // Specifically test that the model works even when execution_status is missing
+     final mockResponseWithoutExecutionStatus = {
+       'finality_status': status.finalityStatus.toString()
+     };
+     final partialStatus = TransactionStatus.fromJson(mockResponseWithoutExecutionStatus);
+     expect(partialStatus.finalityStatus, isNotNull);
+     expect(partialStatus.executionStatus, isNull);
    }, tags: ['integration']);

Also applies to: 196-219


1-221: Consider refactoring HTTP request logic for DRY principle.

The code repeats the pattern of making HTTP requests multiple times. Consider extracting this logic into a helper function to avoid repetition.

Extract a helper function for making HTTP requests:

Future<Map<String, dynamic>> makeRpcRequest(
  Uri uri,
  String method,
  List<dynamic> params,
) async {
  final response = await http.post(
    uri,
    headers: {'Content-Type': 'application/json'},
    body: jsonEncode({
      'jsonrpc': '2.0',
      'method': method,
      'params': params,
      'id': 1,
    }),
  );
  
  print('Response from $method: ${response.body}');
  return jsonDecode(response.body);
}

Then you can simplify the requests in your tests:

// Get chain ID
final chainIdResult = await makeRpcRequest(devnetUri, 'starknet_chainId', []);
expect(chainIdResult['error'], isNull, reason: 'Failed to get chain ID');

// Get the latest nonce
final nonceResult = await makeRpcRequest(
  devnetUri, 
  'starknet_getNonce', 
  ['latest', senderAddress.toHexString()]
);
expect(nonceResult['error'], isNull, reason: 'Failed to get nonce');
🧰 Tools
🪛 GitHub Actions: CI

[warning] 167-167: The '!' will have no effect because the receiver can't be null. Try removing the '!' operator. - unnecessary_non_null_assertion

🪛 GitHub Actions: code_health

[warning] 167-167: The '!' will have no effect because the receiver can't be null. Try removing the '!' operator. - unnecessary_non_null_assertion


40-169:

❓ Verification inconclusive

Fix unnecessary non-null assertions and improve transaction polling.

The test properly sets up a real transaction and checks its status, but there are a couple of improvements needed:

  1. Lines 165 and 167 contain unnecessary non-null assertions (!). Since you already checked that status is not null on lines 163-164, Dart's flow analysis knows status can't be null.

  2. The error handling in the polling loop could be improved to provide more context.

Fix the unnecessary non-null assertions:

-      expect(status!.finalityStatus, isNotNull,
+      expect(status.finalityStatus, isNotNull,
           reason: 'Transaction should have a finality status');
-      expect(status!.executionStatus, isNotNull,
+      expect(status.executionStatus, isNotNull,
           reason: 'Transaction should have an execution status');

Improve error handling in the polling loop:

        } catch (e) {
-          print('Error polling for transaction status: $e');
+          print('Error polling for transaction status: $e');
+          // Continue polling despite errors
        }

🏁 Script executed:

#!/bin/bash
# Check if the pipeline failure is due to the unnecessary non-null assertions
cd packages/starknet_provider
dart analyze test/get_transaction_status_test.dart

Length of output: 179


Action Required: Manual verification needed for non-null assertions and error handling improvements

The test still requires two updates in the file packages/starknet_provider/test/get_transaction_status_test.dart:

  • Remove unnecessary non-null assertions:
    Since the check asserting status is not null (around lines 163–164) guarantees that Dart’s flow analysis treats status as non-null, the explicit non-null assertions (status!) on lines 165 and 167 can be safely removed. The diff snippet should be updated as follows:

    -      expect(status!.finalityStatus, isNotNull,
    +      expect(status.finalityStatus, isNotNull,
             reason: 'Transaction should have a finality status');
    -      expect(status!.executionStatus, isNotNull,
    +      expect(status.executionStatus, isNotNull,
             reason: 'Transaction should have an execution status');
  • Improve error handling in the polling loop:
    While the current handling prints the error, a comment clarifying that polling continues despite errors would be beneficial. For instance:

          } catch (e) {
    -          print('Error polling for transaction status: $e');
    +          print('Error polling for transaction status: $e');
    +          // Continue polling despite errors
          }

The attempt to verify these changes via the automated dart analyze command was inconclusive due to the environment lacking the Dart tool (dart: command not found). Please perform a manual verification to ensure that these changes remove any analyzer warnings and that the error handling meets the intended improvement.

🧰 Tools
🪛 GitHub Actions: CI

[warning] 167-167: The '!' will have no effect because the receiver can't be null. Try removing the '!' operator. - unnecessary_non_null_assertion

🪛 GitHub Actions: code_health

[warning] 167-167: The '!' will have no effect because the receiver can't be null. Try removing the '!' operator. - unnecessary_non_null_assertion

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8613897 and dbcbd77.

📒 Files selected for processing (2)
  • packages/starknet_provider/README.md (1 hunks)
  • packages/starknet_provider/test/get_transaction_status_test.dart (1 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI
packages/starknet_provider/test/get_transaction_status_test.dart

[warning] 167-167: The '!' will have no effect because the receiver can't be null. Try removing the '!' operator. - unnecessary_non_null_assertion

🪛 GitHub Actions: code_health
packages/starknet_provider/test/get_transaction_status_test.dart

[warning] 167-167: The '!' will have no effect because the receiver can't be null. Try removing the '!' operator. - unnecessary_non_null_assertion

🔇 Additional comments (3)
packages/starknet_provider/README.md (1)

30-30: Documentation accurately updated to reflect implementation status.

The README has been correctly updated to show that starknet_getTransactionStatus is now implemented (✅). This aligns with the PR objective of implementing this RPC method.

packages/starknet_provider/test/get_transaction_status_test.dart (2)

8-18: Well-structured test initialization with clear constants.

The constant values for test configuration (polling interval, timeout) are well-defined at the top of the file, making it easy to adjust test parameters. The shared variable for transaction hash is also a good approach for test dependency management.


20-38: Good error case handling for non-existent transaction hash.

This test effectively verifies the error response when a non-existent transaction hash is provided. The error code and message assertions are appropriate.

Copy link
Collaborator

@ptisserand ptisserand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution.
Please update your implementation to use enum for a better match with JSON RPC specification and also use getTransactionStatus method of provider instead of http.post with expected JSON payload

@freezed
class TransactionStatus with _$TransactionStatus {
const factory TransactionStatus({
required String finalityStatus,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to JSON RPC SPEC, finalityStatus should be an enum

class TransactionStatus with _$TransactionStatus {
const factory TransactionStatus({
required String finalityStatus,
String? executionStatus,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to JSON RPC SPEC, executionStatus should be an enum

part 'get_transaction_status.g.dart';

@freezed
class TransactionStatus with _$TransactionStatus {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that error handling is missing


/// Gets the status of a transaction by its hash
///
/// [Spec](https://github.com/starkware-libs/starknet-specs/blob/v0.7.0-rc0/api/starknet_api_openrpc.json#L144-L169)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use permanent link instead

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your tests should use getTransationStatus method instead of http.post with expected JSON payload.

@ptisserand
Copy link
Collaborator

Hi @jaiminRaiyani are you still working on it ?

@jaiminRaiyani
Copy link
Contributor Author

jaiminRaiyani commented May 5, 2025

Hi @jaiminRaiyani are you still working on it ?

nope you can assign this issue to someone else i am not able to solve this with your requested changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants