Skip to content
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

Sdk: expose bridge tx status #1658

Merged
merged 17 commits into from
Dec 18, 2023
Merged

Sdk: expose bridge tx status #1658

merged 17 commits into from
Dec 18, 2023

Conversation

ChiTimesChi
Copy link
Collaborator

@ChiTimesChi ChiTimesChi commented Dec 13, 2023

Description
Added two functions to track the Synapse transactions:

  • getSynapseTxID(originChainId, bridgeModuleName, txHash) returns synapseTxID, which can be used to track the transaction status.
    • If the provided transaction is not an "origin transaction" for the corresponding "bridge module" (SynapseBridge / SynapseCCTP), this will throw an explicit error: 7f92da2
  • getBridgeTxStatus(destChainId, bridgeModuleName, synapseTxID) returns the status of the transaction on the destination chain.
    • This will always return false for unrelated synapseTxID

Additional context
Add any other context about the problem you're solving.

Metadata

  • Fixes #[Link to Issue]

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Added bridge transaction ID retrieval and transaction completion status check functions.
  • Enhancements

    • Improved estimation of bridge operation times for enhanced user experience.
  • Tests

    • Added tests to ensure reliability of new bridge transaction features.
  • Documentation

    • Updated public API documentation to include new bridge transaction tracking functions.

Copy link
Contributor

coderabbitai bot commented Dec 13, 2023

Walkthrough

The SDK has undergone significant changes to enhance the abstraction and standardization of bridge operations. New async functions getSynapseTxId and getBridgeTxStatus have been introduced to handle bridge transaction identification and status checks across various modules. Additionally, abstract methods have been added to the Router and RouterSet classes, and test cases for Bridge Tx Status functionality have been included. These changes aim to provide a more consistent developer experience when interacting with bridge modules.

Changes

File Path Changes
.../operations/bridge.ts
.../router/router.ts
.../router/routerSet.ts
.../router/synapseCCTPRouter.ts
.../router/synapseCCTPRouterSet.ts
.../router/synapseRouter.ts
.../sdk.ts
.../utils/logs.ts
Added new async functions for bridge operations, introduced abstract methods, and added test cases for Bridge Tx Status functionality.

🐇✨
In the world of code, where bridges connect,
New functions arise, with bugs to detect.
A rabbit hops through, with a coder's delight,
"Let's bridge the gaps, and make it all right!"

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on X ?


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • You can reply to a review comment made by CodeRabbit.
  • You can tag CodeRabbit on specific lines of code or files in the PR by tagging @coderabbitai in a comment.
  • You can tag @coderabbitai in a PR comment and ask one-off questions about the PR and the codebase. Use quoted replies to pass the context for follow-up questions.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • 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/coderabbit-overrides.v2.json

Copy link

codecov bot commented Dec 13, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ba6ce85) 51.34720% compared to head (6e72b35) 51.43940%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@                 Coverage Diff                 @@
##              master       #1658         +/-   ##
===================================================
+ Coverage   51.34720%   51.43940%   +0.09219%     
===================================================
  Files            362         363          +1     
  Lines          24755       24802         +47     
  Branches         284         288          +4     
===================================================
+ Hits           12711       12758         +47     
  Misses         10807       10807                 
  Partials        1237        1237                 
Flag Coverage Δ
packages 91.38418% <100.00000%> (+0.61262%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions github-actions bot added size/l and removed size/xs labels Dec 14, 2023
@ChiTimesChi
Copy link
Collaborator Author

ChiTimesChi commented Dec 14, 2023

Ready for the initial review. Open questions so far:

  • Is bridgeID a good naming? The idea is to abstract away the difference between's SynapseBridge kappa and SynapseCCTP requestID from the SDK consumer.
  • Should getBridgeID() for SynapseBridge throw an error, if the provided transaction does not represent a valid origin SynapseBridge transaction?
    • Right now the behavior is that bridgeID is returned which always yields status false in getBridgeTxStatus()
    • Could throw an error to be consistent with SynapseCCTP behavior, but that would require to do one extra getTransactionReceipt RPC call.

@ChiTimesChi ChiTimesChi marked this pull request as ready for review December 14, 2023 12:40
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.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between a17159d and 276f083.
Files ignored due to filter (1)
  • packages/sdk-router/src/abi/SynapseCCTP.json
Files selected for processing (9)
  • packages/sdk-router/src/operations/bridge.ts (3 hunks)
  • packages/sdk-router/src/router/router.ts (1 hunks)
  • packages/sdk-router/src/router/routerSet.ts (1 hunks)
  • packages/sdk-router/src/router/synapseCCTPRouter.ts (3 hunks)
  • packages/sdk-router/src/router/synapseCCTPRouterSet.ts (1 hunks)
  • packages/sdk-router/src/router/synapseRouter.ts (2 hunks)
  • packages/sdk-router/src/router/synapseRouterSet.ts (1 hunks)
  • packages/sdk-router/src/sdk.test.ts (1 hunks)
  • packages/sdk-router/src/sdk.ts (1 hunks)
Additional comments: 32
packages/sdk-router/src/operations/bridge.ts (5)
  • 120-158: The implementation of getBridgeID and getBridgeTxStatus functions correctly uses the getRouterSet helper to abstract the differences between bridge modules and fetch the required data. The async nature of these functions is consistent with the expected behavior of making network requests or other asynchronous operations.

  • 214-222: The getRouterSet helper function is implemented to correctly determine the RouterSet based on the bridgeModuleName. It uses strict equality checks and throws an error for unknown modules, which is a good practice for ensuring the correctness of the module name and fail-fast behavior.

  • 183-197: The modification of the getEstimatedTime function to use the getRouterSet helper function aligns with the PR's objective of enhancing abstraction and standardization of bridge operations. This change ensures consistency in how router sets are retrieved across different operations.

  • 160-162: The getBridgeModuleName function remains unchanged and should continue to function correctly with the new changes, provided that the allEvents property is correctly maintained in the RouterSet instances.

  • 210-212: The getBridgeGas function is not directly affected by the new changes and should continue to function as expected, interacting with the synapseRouterSet to retrieve the chain gas amount.

packages/sdk-router/src/router/router.ts (2)
  • 64-81: The addition of the abstract methods getBridgeID and getBridgeTxStatus to the Router class is consistent with the PR's objectives to enhance the SDK's transaction tracking capabilities across blockchain networks. Ensure that all classes extending Router implement these new methods.

  • 64-81: Verify that all subclasses of Router have been updated to implement the new abstract methods getBridgeID and getBridgeTxStatus.

Verification successful

The new script has successfully identified two classes that extend the Router class: SynapseRouter and SynapseCCTPRouter. Both classes have implemented the new abstract methods getBridgeID and getBridgeTxStatus as required by the changes in the Router class.

  • The SynapseRouter class implements getBridgeID and getBridgeTxStatus methods on lines 144 and 151 respectively.
  • The SynapseCCTPRouter class implements getBridgeID and getBridgeTxStatus methods on lines 123 and 151 respectively.

Based on this information, it can be confirmed that the subclasses of Router have been updated to include the new abstract methods as per the PR's objective.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Find all subclasses of Router and check if they implement getBridgeID and getBridgeTxStatus methods.
ast-grep --lang typescript --pattern $'class $_ extends Router {
  $$$
  getBridgeID($_) {
    $$$
  }
  $$$
  getBridgeTxStatus($_) {
    $$$
  }
  $$$
}'

Length of output: 156



Script:

#!/bin/bash
# Find any class that extends Router
ast-grep --lang typescript --pattern $'class $_ extends Router {
  $$$
}'

Length of output: 27790

packages/sdk-router/src/router/routerSet.ts (3)
  • 74-94: The addition of abstract methods getBridgeID and getBridgeTxStatus in the RouterSet class is consistent with the PR's objectives to enhance the SDK's capabilities for tracking bridge transactions. The method signatures are clear, and the return types are appropriate for the described functionality. Ensure that the implementations of these methods in subclasses handle errors gracefully and provide clear documentation for developers using the SDK.

  • 71-72: The getEstimatedTime method appears unchanged in this hunk. The AI-generated summary mentioned modifications to this method, but they are not reflected here. If there are changes to this method in other parts of the codebase, ensure they are consistent with the SDK's design and correctly documented.

  • 74-94: While the abstract methods getBridgeID and getBridgeTxStatus are added, the PR's objective mentions handling different cases for SynapseCCTP and SynapseBridge transactions. Ensure that the subclasses or other parts of the codebase implement this logic and that it is well-documented for clarity.

Verification successful

The output from the shell script indicates that the terms 'SynapseCCTP' and 'SynapseBridge' are indeed used throughout the codebase, and there are specific classes and test cases that likely contain the logic for handling different cases for SynapseCCTP and SynapseBridge transactions. To verify this, I will generate a script to extract the relevant parts of the code from the SynapseCCTPRouterSet, SynapseRouterSet classes, and the test cases in sdk.test.ts.


The previous script failed because it did not include a pattern for rg to search for. To correct this, I will generate a new script that uses the --files option to list the files and then output their content using xargs cat.


Based on the provided content from the synapseCCTPRouterSet.ts, synapseRouterSet.ts, and sdk.test.ts files, it is clear that the differentiation logic for handling SynapseCCTP and SynapseBridge transactions is implemented in the respective SynapseCCTPRouterSet and SynapseRouterSet classes. The test cases cover the functionality for both types of transactions, confirming that the PR's objective is met and the logic is well-documented and tested.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Search for implementations of getBridgeID and getBridgeTxStatus in subclasses
rg --type ts 'getBridgeID|getBridgeTxStatus' --files-with-matches | xargs rg 'SynapseCCTP|SynapseBridge'

Length of output: 9940



Script:

#!/bin/bash
# Extract the implementation details from SynapseCCTPRouterSet and SynapseRouterSet classes
rg --type ts --glob 'synapseCCTPRouterSet.ts' --glob 'synapseRouterSet.ts' --glob 'sdk.test.ts'

Length of output: 488



Script:

#!/bin/bash
# List the content of the SynapseCCTPRouterSet, SynapseRouterSet classes, and the test cases in sdk.test.ts
rg --type ts --files --glob 'synapseCCTPRouterSet.ts' --glob 'synapseRouterSet.ts' --glob 'sdk.test.ts' | xargs cat

Length of output: 42477

packages/sdk-router/src/router/synapseCCTPRouter.ts (5)
  • 11-11: The import of cctpAbi is correctly added to support the new contract interactions.

  • 31-31: The addition of cctpContractCache as a private property is a good practice to avoid creating a new contract instance on every call.

  • 123-145: The implementation of getBridgeID correctly retrieves the contract instance, fetches the transaction receipt, and parses the logs to find the CircleRequestSent event. Error handling is present to throw an error if the log is not found.

  • 151-153: The getBridgeTxStatus method is concise and correctly uses the cached contract instance to check the transaction status.

  • 156-167: The getCctpContract method correctly implements lazy initialization for the cctpContractCache, ensuring that the contract instance is created only once and reused for subsequent calls.

packages/sdk-router/src/router/synapseCCTPRouterSet.ts (2)
  • 31-49: The implementation of getBridgeID and getBridgeTxStatus methods correctly delegates to the SynapseCCTPRouter instances. Ensure that the base class RouterSet defines these methods to fulfill the @inheritdoc documentation.

  • 28-53: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [51-57]

The use of invariant to ensure the existence of SynapseCCTPRouter instances is a good practice for error handling and should be maintained.

packages/sdk-router/src/router/synapseRouter.ts (5)
  • 6-6: The import of solidityKeccak256 is correctly added to support the new functionality.

  • 141-146: The implementation of getBridgeID correctly computes a hash from the transaction hash using solidityKeccak256.

  • 148-154: The implementation of getBridgeTxStatus correctly checks the existence of a bridge transaction by calling kappaExists on the bridge contract.

  • 158-158: The getBridgeContract method correctly implements caching for the bridge contract instance, which is a good practice for performance optimization.

  • 138-158: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [3-154]

The new methods getBridgeID and getBridgeTxStatus are well integrated with the existing SynapseRouter class and follow consistent coding patterns.

packages/sdk-router/src/router/synapseRouterSet.ts (2)
  • 39-57: The new methods getBridgeID and getBridgeTxStatus correctly delegate the functionality to the SynapseRouter instances. Ensure that the SynapseRouter class has these methods implemented and that they handle the parameters originChainId, txHash, and bridgeID appropriately.

  • 59-61: The error handling in getSynapseRouter using invariant is consistent and provides a clear error message. This is good practice for ensuring that the router exists for the given chain ID before attempting to use it.

packages/sdk-router/src/sdk.test.ts (7)
  • 696-738: The test suite setup for 'Bridge Tx Status' is well-structured, with clear separation of test cases for different bridge modules and transaction directions. This organization aids in readability and maintainability.

  • 738-809: The tests for getBridgeID method are comprehensive, covering both success scenarios and expected failures for invalid inputs. It's good to see that the tests check for specific error messages, which helps ensure that the function is failing for the right reasons.

  • 812-901: Similarly, the tests for getBridgeTxStatus method are thorough, testing for correct status retrieval and handling of invalid inputs. The use of both positive and negative test cases ensures a robust test coverage.

  • 778-785: It's important to verify that the test for the getBridgeID method with an invalid transaction hash correctly simulates the scenario where the transaction does not refer to SynapseCCTP. Ensure that the transaction hash used in the test (bridgeEthToArbTx.txHash) is indeed not related to SynapseCCTP and that the error message 'CircleRequestSent log not found' is accurate and expected in this case.

  • 788-797: For the test case that expects to throw when given a destination transaction hash, ensure that the provided hash (0xefb946d2acf8343ac5526de66de498e0d5f70ae73c81b833181616ee058a22d7) is indeed a destination transaction hash and that the expected behavior of the getBridgeID method is to throw an error in such cases.

Verification successful

The output from the rg command provides the context needed to verify the nature of the transaction hash 0xefb946d2acf8343ac5526de66de498e0d5f70ae73c81b833181616ee058a22d7. The comments in the sdk.test.ts file indicate that this hash is associated with a transaction on Etherscan, which is a block explorer for the Ethereum blockchain. The comment above the test case states "Has CircleRequestFulfilled log, which should be ignored," suggesting that the test case is indeed expecting a destination transaction hash and is designed to test the behavior when such a hash is provided to the getBridgeID method.

Given this context, it is clear that the transaction hash is used in the test to simulate a destination transaction, and the getBridgeID method is expected to throw an error in this case, which is consistent with the test case's assertion that it should throw with the message 'CircleRequestSent log not found'.

Based on this information, the test case appears to be correctly using the transaction hash to verify the expected behavior of the getBridgeID method when provided with a destination transaction hash.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Verify that the transaction hash used in the test is a destination transaction hash
# Expecting to find logs related to SynapseCCTP for this transaction hash indicating it's a destination transaction
ast-grep --lang python --pattern $'synapse.getBridgeID($_, "SynapseCCTP", "0xefb946d2acf8343ac5526de66de498e0d5f70ae73c81b833181616ee058a22d7")'

Length of output: 145



Script:

#!/bin/bash
# Verify that the transaction hash used in the test is a destination transaction hash
# Expecting to find comments or code indicating that the transaction hash is a destination transaction hash
ast-grep --lang javascript --pattern $'synapse.getBridgeID($_, "SynapseCCTP", "0xefb946d2acf8343ac5526de66de498e0d5f70ae73c81b833181616ee058a22d7")'

Length of output: 149



Script:

#!/bin/bash
# Search for the transaction hash in the codebase to verify if it's used as a destination transaction hash
# Include context lines to find comments or code that indicate the nature of the transaction hash
rg --context 5 '0xefb946d2acf8343ac5526de66de498e0d5f70ae73c81b833181616ee058a22d7'

Length of output: 1761

* 832-839: In the test case that checks for a false return value when an unknown `bridgeID` is used, it's using a `txHash` instead of a `bridgeID`. This test should be verified to ensure that the `getBridgeTxStatus` method is expected to return `false` when a `txHash` is passed instead of a `bridgeID`, and that this is the intended behavior.
Verification successful

The test case in question is designed to verify that the getBridgeTxStatus method returns false when an incorrect identifier, such as a txHash, is passed instead of a bridgeID. The implementation of the getBridgeTxStatus method in operations/bridge.ts confirms that it is intended to take a bridgeID as an argument. Therefore, the test case is correctly testing the method's behavior with an invalid argument, and the method is expected to return false in such a scenario.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Verify that the getBridgeTxStatus method should return false when a txHash is passed instead of a bridgeID
# Expecting the method to return false for txHashes and true for valid bridgeIDs
ast-grep --lang python --pattern $'synapse.getBridgeTxStatus($_, "SynapseBridge", "0xb88feb2a92690448b840851dff41dbc7cdc975c1fb740f0523b5c2e407ac9f38")'

Length of output: 153



Script:

#!/bin/bash
# Search for the usage of getBridgeTxStatus method to understand its expected arguments and behavior
ast-grep --lang javascript --pattern $'synapse.getBridgeTxStatus($_, $_, $_)'

Length of output: 78



Script:

#!/bin/bash
# Use ripgrep to search for the usage of getBridgeTxStatus method in the codebase
rg "getBridgeTxStatus" --files-with-matches

Length of output: 446



Script:

#!/bin/bash
# Extract the implementation of getBridgeTxStatus method from sdk.ts
rg "getBridgeTxStatus" packages/sdk-router/src/sdk.ts -A 10 -B 10

Length of output: 901



Script:

#!/bin/bash
# Extract the implementation of getBridgeTxStatus method from operations/bridge.ts
rg "getBridgeTxStatus" packages/sdk-router/src/operations/bridge.ts -A 10 -B 10

Length of output: 986

* 842-849: The test case that checks for a false return value when the origin chain is used instead of the destination is important for validating the method's logic. Confirm that the `getBridgeTxStatus` method should indeed return `false` when the origin chain ID is provided as the first argument instead of the destination chain ID.
packages/sdk-router/src/sdk.ts (1)
  • 51-52: The addition of getBridgeID and getBridgeTxStatus methods to the SynapseSDK class is consistent with the PR's objective to enhance bridge transaction tracking capabilities. Ensure that the implementations of these methods in ./operations are thoroughly tested and handle all edge cases as they are now part of the public API.

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.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 276f083 and cf2b7e5.
Files selected for processing (2)
  • packages/sdk-router/src/router/synapseCCTPRouter.ts (3 hunks)
  • packages/sdk-router/src/sdk.test.ts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
  • packages/sdk-router/src/router/synapseCCTPRouter.ts
  • packages/sdk-router/src/sdk.test.ts

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.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between cf2b7e5 and cd19b3b.
Files selected for processing (4)
  • packages/sdk-router/src/router/synapseCCTPRouter.ts (3 hunks)
  • packages/sdk-router/src/router/synapseRouter.ts (4 hunks)
  • packages/sdk-router/src/sdk.test.ts (1 hunks)
  • packages/sdk-router/src/utils/logs.ts (1 hunks)
Files skipped from review as they are similar to previous changes (3)
  • packages/sdk-router/src/router/synapseCCTPRouter.ts
  • packages/sdk-router/src/router/synapseRouter.ts
  • packages/sdk-router/src/sdk.test.ts
Additional comments: 1
packages/sdk-router/src/utils/logs.ts (1)
  • 1-52: The code in logs.ts provides utility functions for extracting logs from transaction receipts and appears to be correct and efficient. The functions are well-documented, handle errors appropriately, and the logic is sound. There are no apparent issues with security, performance, or logic. Ensure that these utility functions are integrated properly with the new features introduced in the PR to track bridge transaction statuses.

@ChiTimesChi
Copy link
Collaborator Author

ChiTimesChi commented Dec 14, 2023

The previous comment was partly resolved in cd19b3b

@Defi-Moses
Copy link
Collaborator

bridgeID works but doesn't intuitively tell me that its asking for the unique transaction identifier. I think something along the lines of transactionID or synapseID. I like something along the lines of synapseTxId because its clear but might be a little bit wordy.

@bigboydiamonds
Copy link
Collaborator

bridgeID works but doesn't intuitively tell me that its asking for the unique transaction identifier. I think something along the lines of transactionID or synapseID. I like something along the lines of synapseTxId because its clear but might be a little bit wordy.

Agreed that bridgeID could be confusing whether that refers to transactionHash or kappa when calling SDK from frontend

Adding a few suggestions for consideration:

getBridgeKappa() => returns bridgeKappa
getBridgeTxnKappa() => returns bridgeTxnKappa

@ChiTimesChi
Copy link
Collaborator Author

kappa is specific to the SynapseBridge, which is only one of the solutions that SDK is supposed to abstract away from the end consumer, so it doesn't feel too nice.

I like synapseTxId over transactionId, as the latter could be easily confused with transactionHash, while the former is quite explicit.

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.

Review Status

Actionable comments generated: 4

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between cd19b3b and b62d7a1.
Files selected for processing (9)
  • packages/sdk-router/src/operations/bridge.ts (3 hunks)
  • packages/sdk-router/src/router/router.ts (1 hunks)
  • packages/sdk-router/src/router/routerSet.ts (1 hunks)
  • packages/sdk-router/src/router/synapseCCTPRouter.ts (3 hunks)
  • packages/sdk-router/src/router/synapseCCTPRouterSet.ts (1 hunks)
  • packages/sdk-router/src/router/synapseRouter.ts (4 hunks)
  • packages/sdk-router/src/router/synapseRouterSet.ts (1 hunks)
  • packages/sdk-router/src/sdk.test.ts (1 hunks)
  • packages/sdk-router/src/sdk.ts (1 hunks)
Additional comments: 22
packages/sdk-router/src/operations/bridge.ts (2)
  • 120-158: The addition of getSynapseTxId and getBridgeTxStatus functions aligns with the PR's objective to enhance transaction tracking across blockchain networks. The implementation appears to correctly delegate to the getRouterSet helper function to abstract away differences between bridge modules. Ensure that the getRouterSet function is thoroughly tested, especially since it's used by multiple new functions and any issues could have widespread effects.

  • 183-197: The refactoring of getEstimatedTime to use the getRouterSet helper function is a good example of code reuse and simplification. Ensure that this change has been tested across all scenarios where getEstimatedTime is used, including edge cases where the bridge module name might be unknown or incorrect.

packages/sdk-router/src/router/router.ts (1)
  • 64-81: Please ensure that all concrete subclasses of the Router class have implemented the new abstract methods getSynapseTxId and getBridgeTxStatus. This is necessary to maintain the integrity of the class hierarchy and to ensure that the new functionality is available across all router types.
Verification successful

The script provided did not yield any results, which could indicate that the new abstract methods getSynapseTxId and getBridgeTxStatus have not been implemented in any subclasses of Router. However, before concluding, I should verify if the script was correct and potentially run a new script to confirm the implementation status of these methods.

I will now generate a new script to check for the presence of these methods in the subclasses of Router. If the methods are not implemented, this would be a serious issue as it would break the contract established by the abstract class Router.


The verification process has confirmed that the new abstract methods getSynapseTxId and getBridgeTxStatus have been implemented in the subclasses of the Router class. The methods are present in synapseCCTPRouter.ts, synapseCCTPRouterSet.ts, synapseRouter.ts, and synapseRouterSet.ts, as required to maintain the integrity of the class hierarchy and provide the new functionality across all router types.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Search for subclasses of `Router` and check if they implement the new methods.
ast-grep --lang typescript --pattern $'class $_ extends Router {
  $$$
  getSynapseTxId($_) {
    $$$
  }
  getBridgeTxStatus($_) {
    $$$
  }
  $$$
}'

Length of output: 153



Script:

#!/bin/bash
# Search for any TypeScript files that contain classes extending `Router`
# and then check if the new methods `getSynapseTxId` and `getBridgeTxStatus` are implemented.
rg --type ts 'class .* extends Router' -l | xargs rg --type ts 'getSynapseTxId|getBridgeTxStatus'

Length of output: 2183

packages/sdk-router/src/router/routerSet.ts (1)
  • 74-97: The addition of the abstract methods getSynapseTxId and getBridgeTxStatus to the RouterSet class aligns with the PR's objective to enhance transaction tracking capabilities. Ensure that all subclasses of RouterSet implement these new methods to maintain the integrity of the class hierarchy.
Verification successful

The previous script did not find any subclasses of RouterSet implementing the getSynapseTxId and getBridgeTxStatus methods. To ensure we have accurate information, let's broaden the search to identify any subclasses of RouterSet and then check if they implement these methods.


The subclasses SynapseRouterSet and SynapseCCTPRouterSet of RouterSet have implemented the new methods getSynapseTxId and getBridgeTxStatus as required. This confirms that the PR's changes are consistent with its objectives to enhance transaction tracking capabilities across blockchain networks.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Search for all subclasses of RouterSet and ensure they implement the new methods.
ast-grep --lang typescript --pattern $'class $_ extends RouterSet {
  $$$
  getSynapseTxId($_, $_) {
    $$$
  }
  $$$
  getBridgeTxStatus($_, $_) {
    $$$
  }
  $$$
}'

Length of output: 170



Script:

#!/bin/bash
# Search for all subclasses of RouterSet without specifying the methods.
ast-grep --lang typescript --pattern $'class $_ extends RouterSet {
  $$$
}'

Length of output: 9938

packages/sdk-router/src/router/synapseCCTPRouter.ts (7)
  • 11-12: The addition of imports for cctpAbi and getMatchingTxLog aligns with the PR's objective to enhance transaction tracking capabilities.

  • 32-35: The addition of the private member cctpContractCache and the originEvents array is consistent with the PR's goal to manage contract instances and event tracking.

  • 127-137: The implementation of getSynapseTxId correctly uses the getCctpContract method to retrieve the contract instance and getMatchingTxLog to fetch the transaction log.

  • 143-145: The getBridgeTxStatus method correctly uses the getCctpContract method to check the transaction status, which aligns with the PR's objectives.

  • 148-159: The getCctpContract method is implemented with caching to optimize contract instance retrieval, which is a good practice for performance.

  • 127-146: The new public methods getSynapseTxId and getBridgeTxStatus are correctly exposed and implemented, fulfilling the PR's objectives.

  • 127-146: Ensure that proper error handling is in place for the new methods getSynapseTxId and getBridgeTxStatus, either through try-catch blocks or by handling rejected promises at the call sites.

packages/sdk-router/src/router/synapseCCTPRouterSet.ts (2)
  • 31-49: The conversion of getSynapseTxId and getBridgeTxStatus methods to asynchronous ones is correctly implemented. The methods now return promises, which is consistent with the PR's objective to handle asynchronous operations for obtaining transaction IDs and checking transaction status.

  • 31-49: There appears to be a discrepancy between the PR's stated objective and the method names in the code. The PR objective mentions a getBridgeID function, but the hunk shows a getSynapseTxId method. Please verify that the method names align with the intended functionality and naming conventions of the SDK.

packages/sdk-router/src/router/synapseRouter.ts (5)
  • 6-6: The addition of the solidityKeccak256 import aligns with the new functionality introduced in the getSynapseTxId method.

  • 28-28: The addition of the getMatchingTxLog import is necessary for the new transaction log matching functionality in the getSynapseTxId method.

  • 64-72: The originEvents array is a new private member that stores event names related to the origin transactions. Ensure that all events listed are relevant and complete.

  • 152-166: The getSynapseTxId method correctly uses the getMatchingTxLog function to ensure the transaction hash refers to an origin transaction before calculating the Synapse txId.

  • 168-174: The getBridgeTxStatus method implementation is consistent with its intended functionality to check the transaction status using the bridgeContract.

packages/sdk-router/src/sdk.test.ts (3)
  • 696-965: The new test cases added for getSynapseTxId and getBridgeTxStatus methods are comprehensive and cover a variety of scenarios, including error handling and successful retrievals. This is a good practice to ensure the robustness of the new functionality.

  • 702-736: The test cases rely on hardcoded transaction hashes and Synapse transaction IDs. Ensure that these transactions are stable and won't change over time, as they are used as fixtures for the test cases.

  • 762-870: The inclusion of error handling tests for non-existent transaction hashes and incorrect bridge module names is commendable. It ensures that the SDK can handle unexpected conditions gracefully.

packages/sdk-router/src/sdk.ts (1)
  • 52-52: The addition of the getBridgeTxStatus method aligns with the PR objectives and enhances the SDK's functionality as intended.

packages/sdk-router/src/sdk.test.ts Show resolved Hide resolved
packages/sdk-router/src/operations/bridge.ts Outdated Show resolved Hide resolved
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.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between b62d7a1 and 6e72b35.
Files selected for processing (2)
  • packages/sdk-router/src/operations/bridge.ts (3 hunks)
  • packages/sdk-router/src/sdk.test.ts (3 hunks)
Files skipped from review due to trivial changes (1)
  • packages/sdk-router/src/operations/bridge.ts
Additional comments: 3
packages/sdk-router/src/sdk.test.ts (3)
  • 874-963: The test cases for getBridgeTxStatus are correctly asserting the boolean status as per the PR specification and the user's clarification.

  • 1297-1319: The tests for the internal function getRouterSet correctly verify the returned router set based on the bridge module name.

  • 694-969: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [697-1320]

The tests added for the new getBridgeID and getBridgeTxStatus methods are comprehensive and appear to cover various scenarios effectively.

@ChiTimesChi ChiTimesChi merged commit 2e9e93f into master Dec 18, 2023
35 checks passed
@ChiTimesChi ChiTimesChi deleted the sdk/bridge-tx-status branch December 18, 2023 18:52
aureliusbtc added a commit that referenced this pull request Dec 18, 2023
* Fix/update wagmi main (#1665)

* Update wagmi to 1.4.12

* yarn install

* Publish

 - @synapsecns/[email protected]

* Sdk: expose bridge tx status (#1658)

* Add placeholder logic for new functions

* Chore: fix spelling

* Add tests with expected behavior

* Generalize bridgeModule -> RouterSet retrieval

* Add skeleton for the end implementation

* Implement ID / status for SynapseBridge

* CCTP status check

* Define behavior for destination CCTP tx

* SynapseCCTP: getBridgeID

* Add test for incorrect tx hash

* Check that transaction receipt is not null

* Tests for generalized "incorrect origin tx" behavior

* Generalized tool for log extraction

* Adapt synapseCCTP, add check to synapseBridge

* Chore: bridgeId -> synapseTxId

* Extra coverage to keep Code Rabbi happy

* Publish

 - @synapsecns/[email protected]
 - @synapsecns/[email protected]
 - @synapsecns/[email protected]

* Update polling time

* Update polling time

* Add constant for polling interval duration

---------

Co-authored-by: χ² <[email protected]>
Co-authored-by: ChiTimesChi <[email protected]>
Co-authored-by: aureliusbtc <[email protected]>
Defi-Moses added a commit that referenced this pull request Dec 19, 2023
* refactoring old explorer to make updates easier

* final restructuring

* fixing yarn build and getting rid of an unused file

* Fix/update wagmi main (#1665)

* Update wagmi to 1.4.12

* yarn install

* Publish

 - @synapsecns/[email protected]

* adding yarn

* switches explorer to use snowtrace on explorer and FE

* Sdk: expose bridge tx status (#1658)

* Add placeholder logic for new functions

* Chore: fix spelling

* Add tests with expected behavior

* Generalize bridgeModule -> RouterSet retrieval

* Add skeleton for the end implementation

* Implement ID / status for SynapseBridge

* CCTP status check

* Define behavior for destination CCTP tx

* SynapseCCTP: getBridgeID

* Add test for incorrect tx hash

* Check that transaction receipt is not null

* Tests for generalized "incorrect origin tx" behavior

* Generalized tool for log extraction

* Adapt synapseCCTP, add check to synapseBridge

* Chore: bridgeId -> synapseTxId

* Extra coverage to keep Code Rabbi happy

* Publish

 - @synapsecns/[email protected]
 - @synapsecns/[email protected]
 - @synapsecns/[email protected]

* updating yarn file to fix linting tests

* lint

* revert viem

* yarn

* Use Chain from viem

---------

Co-authored-by: χ² <[email protected]>
Co-authored-by: ChiTimesChi <[email protected]>
Co-authored-by: bigboydiamonds <[email protected]>
ChiTimesChi added a commit that referenced this pull request Dec 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants