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

XRP EdgeTxActionSwap Direction Fix #657

Merged
merged 1 commit into from
Nov 2, 2023
Merged

Conversation

Jon-edge
Copy link
Contributor

@Jon-edge Jon-edge commented Nov 1, 2023

Fix for only the bugged src/dest direction fix.

Partial fill amounts that happen after the initial order tx will require
a future fix and api change.

CHANGELOG

Does this branch warrant an entry to the CHANGELOG?

  • Yes
  • No

Dependencies

none

Description

none

Copy link
Member

@paullinator paullinator left a comment

Choose a reason for hiding this comment

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

Approved but just requesting a couple small changes to clean up the code

} else {
const builtinToken = this.builtinTokens[tokenId]
if (builtinToken == null) return
takerDenom = this.builtinTokens[tokenId]?.denominations[0]
Copy link
Member

Choose a reason for hiding this comment

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

change to
takerDenom = builtInToken.denominations[0]

This will fix typescript issue

if (takerDenom == null) {
this.error(`parseRippleDexTxAmount: Unknown denom ${currency}`)
// Shouldn't happen
Copy link
Member

Choose a reason for hiding this comment

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

This should be able to be removed after the change above

Fix for only the bugged src/dest direction fix.

Partial fill amounts that happen after the initial order tx will require
a future fix and api change.
@Jon-edge Jon-edge force-pushed the jon/fix/xrp-txaction branch from 6126a12 to 507a5ac Compare November 2, 2023 19:44
@Jon-edge Jon-edge enabled auto-merge November 2, 2023 19:48
@Jon-edge Jon-edge merged commit ef852f6 into master Nov 2, 2023
@Jon-edge Jon-edge deleted the jon/fix/xrp-txaction branch November 2, 2023 19:52
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