Skip to content

[XRP] Add support for Escrow transactions #3572

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Dec 8, 2023
Merged

[XRP] Add support for Escrow transactions #3572

merged 7 commits into from
Dec 8, 2023

Conversation

@rikublock
Copy link
Contributor

Description

Add support for the following XRP Ledger transaction types:

How to test

Types of changes

  • New feature (non-breaking change which adds functionality)

Open questions

  • Running clang-format would introduce a lot of unrelated changes as the files weren't formatted properly to being with. Apply anyway?
  • What are your naming conventions for class variables? The current implementation is shadowing declarations.

Checklist

  • Create pull request as draft initially, unless its complete.
  • Add tests to cover changes as needed.
  • Update documentation as needed.
  • If there is a related Issue, mention it in the description.
  • Linting, code formatting

satoshiotomakan
satoshiotomakan requested changes Nov 24, 2023
View reviewed changes
Copy link
Collaborator

@satoshiotomakan satoshiotomakan left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution! Only one note

tests/chains/XRP/TWAnySignerTests.cpp
}

TEST(TWAnySignerRipple, SignEscrowCreate) {
// https://testnet.xrpl.org/transactions/3F581927C742D5FAE65FB0759D0F04EF3B64B4A087911B07975816ECCB59915B
Copy link
Collaborator

Choose a reason for hiding this comment

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

Transaction not found. Also could you please broadcast the transactions on mainnet?
The checklist contains the step to broadcast at least one transaction on mainnet:
https://developer.trustwallet.com/developer/wallet-core/newblockchain#blockchain-checklist

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The transactions are definitely there. Maybe a temporary problem with the explorer? Seems to work now.
I assumed the mainnet transaction would only be necessary when adding an entirely new blockchain, but I'll see what I can do about that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Non of the links in the PR still do not work for me
https://testnet.xrpl.org/transactions/3F581927C742D5FAE65FB0759D0F04EF3B64B4A087911B07975816ECCB59915B

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems indeed unreliable. Depending on which backend node is selected, the transaction history might not be available. I added a bunch of main net transactions, hope that helps.

@satoshiotomakan satoshiotomakan marked this pull request as ready for review December 7, 2023 16:16
@satoshiotomakan
Copy link
Collaborator

Hi @rikublock, I made this PR ready to review and merged master to run the CI checks

@satoshiotomakan
Copy link
Collaborator

@rikublock could you please update com.trustwallet:wallet-core-kotlin to 4.0.10 like at #3574? This is needed to fix the Kotlin Multiplatform CI

satoshiotomakan
satoshiotomakan approved these changes Dec 8, 2023
View reviewed changes
Copy link
Collaborator

@satoshiotomakan satoshiotomakan left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for the contribution!

@Milerius Milerius merged commit 230e7ab into trustwallet:master Dec 8, 2023
zkrypt-crossbar pushed a commit to Cramiumlabs/wallet-core that referenced this pull request Jun 15, 2025
@rikublock @satoshiotomakan
[XRP] Add support for Escrow transactions (trustwallet#3572)
178918a 
* add XRP escrow proto defs

* XRP escrow transaction encoding

* update XRP signer

* XRP escrow transaction tests

* add XRP mainnet escrow transaction tests

* [KMP]: Bump to 4.0.10

---------

Co-authored-by: satoshiotomakan <127754187+satoshiotomakan@users.noreply.github.com>
zkrypt-crossbar pushed a commit to Cramiumlabs/wallet-core that referenced this pull request Jun 21, 2025
@rikublock @satoshiotomakan
[XRP] Add support for Escrow transactions (trustwallet#3572)
271e75c 
* add XRP escrow proto defs

* XRP escrow transaction encoding

* update XRP signer

* XRP escrow transaction tests

* add XRP mainnet escrow transaction tests

* [KMP]: Bump to 4.0.10

---------

Co-authored-by: satoshiotomakan <127754187+satoshiotomakan@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@satoshiotomakan satoshiotomakan satoshiotomakan approved these changes

@lamafab lamafab Awaiting requested review from lamafab

+1 more reviewer

@Milerius Milerius Milerius approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rikublock @satoshiotomakan @Milerius