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

fix: resolve TransactionStream and Ledger model issues #2779

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

Conversation

mvadari
Copy link
Collaborator

@mvadari mvadari commented Sep 17, 2024

High Level Overview of Change

Title says it all.

Context of Change

Fixes #2767
Fixes #2714
Fixes #2715

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

Did you update HISTORY.md?

  • Yes

Test Plan

The models match the output. CI passes.

@mvadari mvadari changed the title fix: fix TransactionStream and Ledger model issues fix: resolve TransactionStream and Ledger model issues Sep 18, 2024
@justinr1234
Copy link
Collaborator

@coderabbitai full review

Copy link

coderabbitai bot commented Sep 19, 2024

Actions performed

Full review triggered.

Copy link

coderabbitai bot commented Sep 19, 2024

Walkthrough

The recent updates to the xrpl package introduce several enhancements, including the addition of a utility function parseTransactionFlags, updates to the TransactionStream and Ledger models to include a new field close_time_iso, and modifications to support API version 2. These changes improve the handling of transaction data and enhance the clarity of time-related information across the package.

Changes

File(s) Change Summary
packages/xrpl/HISTORY.md Added a summary of changes including new utility function and model updates.
packages/xrpl/src/client/partialPayment.ts Updated handleStreamPartialPayment to accept both TransactionStream and TransactionV1Stream.
packages/xrpl/src/models/ledger/Ledger.ts Added close_time_iso property to BaseLedger interface.
packages/xrpl/src/models/methods/index.ts Introduced TransactionV1Stream entity for enhanced streaming capabilities.
packages/xrpl/src/models/methods/subscribe.ts Created TransactionStreamBase interface for API version handling and added close_time_iso.
packages/xrpl/test/fixtures/requests/hashLedger.json Added close_time_iso field to the JSON structure for standardized date format.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant API
    participant TransactionStream
    participant Ledger

    Client->>API: Request transaction stream
    API->>TransactionStream: Provide transaction data
    TransactionStream->>Ledger: Include close_time_iso
    Ledger-->>API: Return ledger data with close_time_iso
    API-->>Client: Send transaction stream with close_time_iso
Loading

Assessment against linked issues

Objective Addressed Explanation
TransactionStream type is wrong when using API version 2 (#2767)
TransactionStream missing close_time_iso field (#2714)
Response of ledger command missing close_time_iso field (#2715)

Suggested labels

dependencies

Suggested reviewers

  • justinr1234
  • ckeshava

Poem

🐇 In the land of code where rabbits hop,
New flags and streams make our hearts stop!
With ISO times shining bright,
Transactions dance in the moonlight.
Hooray for changes, let’s all cheer,
For the XRPL's future is looking clear! 🌟


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 your favorite social media?

❤️ Share
🪧 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 resolve resolve all the CodeRabbit review comments.
  • @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.

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

@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

Outside diff range and nitpick comments (1)
packages/xrpl/HISTORY.md (1)

Line range hint 1-9: The unreleased changes section looks good! Consider adding a doc link for parseTransactionFlags.

The CHANGELOG entry follows the common conventions:

  • Unreleased changes come before the latest release
  • New features and fixes are listed with sub-bullets
  • GitHub references are included

Suggestion: Add a link to the parseTransactionFlags docs or implementation so readers can easily find more details on this new utility function.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between f3960c3 and 2f4f512.

Files selected for processing (6)
  • packages/xrpl/HISTORY.md (1 hunks)
  • packages/xrpl/src/client/partialPayment.ts (2 hunks)
  • packages/xrpl/src/models/ledger/Ledger.ts (1 hunks)
  • packages/xrpl/src/models/methods/index.ts (2 hunks)
  • packages/xrpl/src/models/methods/subscribe.ts (4 hunks)
  • packages/xrpl/test/fixtures/requests/hashLedger.json (1 hunks)
Files skipped from review due to trivial changes (1)
  • packages/xrpl/test/fixtures/requests/hashLedger.json
Additional comments not posted (10)
packages/xrpl/src/models/ledger/Ledger.ts (1)

34-38: LGTM!

The addition of the close_time_iso property to the BaseLedger interface is a valuable enhancement that provides a standardized way to represent the closure time of the ledger. This change aligns with the PR objectives and resolves the linked issue #2715.

The property is useful for applications that require precise time information in a universally recognized format, and it does not introduce any breaking changes or new issues.

packages/xrpl/src/client/partialPayment.ts (1)

163-166: LGTM!

The changes to the handleStreamPartialPayment function signature and the use of the nullish coalescing operator improve its flexibility and robustness in handling different transaction stream formats. The core functionality of detecting partial payments remains intact, and the updates do not introduce any apparent issues.

packages/xrpl/src/models/methods/index.ts (2)

171-171: LGTM!

The import statement for TransactionV1Stream is correctly added.


587-587: LGTM!

The export statement for TransactionV1Stream is correctly added and is consistent with the import statement.

packages/xrpl/src/models/methods/subscribe.ts (4)

Line range hint 269-306: Excellent work on making the TransactionStreamBase interface more flexible and adaptable!

The introduction of the generic parameter Version allows the interface to adapt based on the API version, which is a great way to ensure compatibility with different versions of the rippled API.

The addition of the close_time_iso property is a nice touch, as it provides a standardized timestamp for the approximate ledger close time.

Making the transaction and tx_json properties conditional based on the Version parameter is a smart move. It ensures that the correct transaction format is used depending on the API version, preventing potential type mismatches.

Overall, these changes enhance the flexibility and robustness of the TransactionStreamBase interface.


321-321: Good simplification of the TransactionStream type.

Aliasing TransactionStream to TransactionStreamBase without specifying the Version generic parameter simplifies the usage of the type. It assumes the default API version, which is a reasonable default for most use cases.

This change makes the type more concise and easier to use.


328-328: Great addition of the TransactionV1Stream type!

The introduction of TransactionV1Stream provides a clear and specific type for handling transaction stream responses from API version 1. By leveraging the generic TransactionStreamBase interface and setting the Version parameter to RIPPLED_API_V1, it ensures that the correct transaction format is used for version 1, preventing potential type mismatches.

This new type enhances the type safety and clarity of the codebase when dealing with version 1 transaction streams.


Line range hint 400-410: Nice addition of the ModifiedOfferCreateTransaction interface.

The ModifiedOfferCreateTransaction interface represents a modified version of the OfferCreate transaction by extending the OfferCreate interface and adding a new owner_funds property.

The owner_funds property provides the numeric amount of the TakerGets currency that the account sending the OfferCreate transaction has after executing the transaction. This information is valuable for tracking and analyzing the account's funds in the context of offer creation.

The addition of this interface enhances the expressiveness and usefulness of the code when dealing with modified offer create transactions.

packages/xrpl/HISTORY.md (2)

10-13: Reviewed the TransactionStream and Ledger model fixes. The changes look good!

The fixes appropriately update the TransactionStream and Ledger models:

  • TransactionStream now supports APIv2 and includes the close_time_iso field
  • Ledger model now includes the missing close_time_iso field

These changes align the models with the latest rippled API and fill in missing data.


Line range hint 1-24: The CHANGELOG looks excellent overall! It's well-structured and provides a clear summary of changes.

Positive aspects of this CHANGELOG:

  • Follows common conventions like reverse chronological ordering
  • Provides version headings with dates for easy scanning
  • Uses consistent formatting for each release section
  • Includes concise, informative bullet points for changes
  • References relevant GitHub issues/PRs for more context
  • Puts an unreleased section at the top for visibility

The changes themselves seem beneficial, with new features, bug fixes, and various improvements that are clearly summarized.

No major issues stood out during the review. This appears to be a thorough, well-maintained CHANGELOG. Nice work!

packages/xrpl/src/client/partialPayment.ts Outdated Show resolved Hide resolved
@ckeshava
Copy link
Collaborator

The CodeRabbit AI does not make a mention of the changes pertaining to tx_json and transaction fields.

I also don't understand the diagram because TransactionStream and API do not refer to anything concrete in the architecture of XRPL.

@mvadari
Copy link
Collaborator Author

mvadari commented Sep 26, 2024

The CodeRabbit AI does not make a mention of the changes pertaining to tx_json and transaction fields.

I also don't understand the diagram because TransactionStream and API do not refer to anything concrete in the architecture of XRPL.

CodeRabbit isn't the smartest. cc @justinr1234

Copy link

@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

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 2f4f512 and 69828a6.

📒 Files selected for processing (2)
  • packages/xrpl/HISTORY.md (1 hunks)
  • packages/xrpl/src/client/partialPayment.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (9)
packages/xrpl/src/client/partialPayment.ts (3)

7-7: LGTM: Addition of TransactionV1Stream type

The import of TransactionV1Stream type is a good addition. It aligns with the PR objectives to address issues related to the TransactionStream model and provides support for both current and legacy versions of the transaction stream.


163-166: LGTM: Improved handling of different transaction stream versions

The changes to the handleStreamPartialPayment function are well-implemented:

  1. The function signature now accepts both TransactionStream and TransactionV1Stream types, improving backwards compatibility.
  2. The use of the nullish coalescing operator (??) in the isPartialPayment function call elegantly handles both tx_json and transaction properties, accommodating different API versions.

These changes align well with the PR objectives to resolve TransactionStream model issues.

Regarding the past review comment, the suggested change has already been implemented in this version of the code.


Line range hint 1-180: Overall assessment: Well-implemented changes addressing TransactionStream issues

The changes in this file effectively address the issues related to the TransactionStream model as outlined in the PR objectives. The implementation:

  1. Adds support for both current and legacy transaction stream types.
  2. Improves backwards compatibility.
  3. Handles different API versions gracefully.

These modifications enhance the flexibility and robustness of the partial payment handling logic without introducing any apparent issues.

packages/xrpl/HISTORY.md (6)

Line range hint 16-46: Version 1.3.0 brings significant changes and new features

This release includes several important updates:

  1. Changes to error handling in prepare* methods:

    • These methods now always reject the Promise when an error occurs, instead of throwing synchronously.
    • This change improves consistency in error handling and allows for better error management in asynchronous code.
  2. Improved RippledError messages:

    • The error_message field is now used as the message, providing more specific error information.
  3. prepareTransaction now allows manual setting of the Sequence field:

    • This gives users more control over transaction sequencing.
  4. Support for rippled 1.2.1 features:

    • Added delivered_amount field to the ledger method and transaction subscriptions.
    • Support for Ed25519 seeds encoded using ripple-lib.

These changes may require updates to existing code, particularly in error handling and transaction preparation.


Line range hint 48-67: Version 1.3.1 includes minor improvements and bug fixes

Notable changes in this version:

  1. Added getLedgerSequence to Remote.
  2. Improved randomness for ECDSA signature generation, enhancing security.
  3. Performance improvement for SerializedObject.append.
  4. Added Amount.scale for multiplying an amount's value by a scale factor.
  5. Changed dropsToXRP and Client.getXrpBalance to return a number instead of a string.
  6. Replaced Buffer with UInt8Array for params and return values.

These changes improve functionality and performance but may require code adjustments, especially for the Buffer to UInt8Array change.


Line range hint 69-96: Version 1.3.2 brings important updates and deprecations

Key changes in this version:

  1. Added support for the ExpandedSignerList amendment.
  2. New fields added to ValidationStream interface.
  3. Improved memo field format checking with more detailed error messages.
  4. Exported verifyKeypairSignature function for use in web apps.
  5. Fixed issues with Wallet.fromMnemonic for RFC1751 mnemonics and invalid encoding detection.
  6. Improved error verbosity in submitAndWait.

Developers should pay attention to these changes, especially the improvements in error handling and new functionality. The expanded signer list support may be particularly relevant for multi-signature setups.


Line range hint 98-116: Version 1.3.3 introduces new features and bug fixes

Notable updates:

  1. Added support for the WalletLocator field.
  2. New helper methods:
    • For creating cross-chain payments to/from a sidechain.
    • For parsing an NFTokenID.
  3. Fixed issues with NFT fields to match rippled 1.9.0 changes.
  4. Updated the TrustSet transaction type, specifically the LimitAmount property type.

These changes enhance the library's capabilities for working with new XRP Ledger features like NFTs and cross-chain payments. Developers working with these features should update to take advantage of the new functionality.


Line range hint 118-129: Versions 1.3.4 and 1.4.0 bring important updates

1.3.4 changes:

  • Fixed a bug in submitAndWait function related to transaction queuing.

1.4.0 changes:

  1. Added support for Node.js v10 LTS.
  2. Introduced DepositPreauth functionality.
  3. Fixed typing issues and improved documentation.
  4. Added getNFTokenID function for retrieving NFTokenID after minting.
  5. Added support for disallowIncoming account set flags.

These versions introduce valuable features and fix important bugs. The support for Node.js v10 LTS and the new DepositPreauth functionality are particularly noteworthy.


Line range hint 1-129: Summary of recent xrpl.js updates

The recent versions of xrpl.js (1.2.5 to 1.4.0) have introduced numerous important changes:

  1. Critical bug fixes, especially in transaction signing and error handling.
  2. New features supporting recent XRP Ledger amendments (e.g., ExpandedSignerList, DepositPreauth).
  3. Improved support for NFTs and cross-chain payments.
  4. Enhanced error reporting and handling.
  5. Performance improvements and modernization of the codebase.
  6. Better TypeScript support and documentation updates.

These changes significantly enhance the library's capabilities and reliability. Users of xrpl.js should consider upgrading to the latest version to benefit from these improvements and to ensure compatibility with the latest XRP Ledger features. However, they should be aware of potential breaking changes, particularly in error handling and data type representations, which may require updates to existing code.

packages/xrpl/HISTORY.md Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants