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.

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

Use Hex for arguments and return values #709

Closed
wants to merge 26 commits into from
Closed

Use Hex for arguments and return values #709

wants to merge 26 commits into from

Conversation

tomaszslabon
Copy link
Contributor

@tomaszslabon tomaszslabon commented Oct 13, 2023 •
edited
Loading

Refs: #713
This PR replaces string with Hex for function arguments and return values in order to unify library interface.

@tomaszslabon tomaszslabon changed the base branch from main to ts-api-revamp October 13, 2023 08:31
@tomaszslabon tomaszslabon self-assigned this Oct 13, 2023
@tomaszslabon tomaszslabon added the 🔌 typescript TypeScript library label Oct 13, 2023
@lukasz-zimnoch lukasz-zimnoch mentioned this pull request Oct 16, 2023
Closed
Base automatically changed from ts-api-revamp to main October 16, 2023 13:29
@tomaszslabon tomaszslabon marked this pull request as ready for review October 20, 2023 16:20
@tomaszslabon
Copy link
Contributor Author

@lukasz-zimnoch
I went through all review comments in #705 and it seems that all the comments related to using Hex are addressed in this PR.
Also, this PR affects the refund script. I executed the refund script with some test data and checked that it works with the changes introduced in this PR.

lukasz-zimnoch
lukasz-zimnoch reviewed Oct 24, 2023
View reviewed changes
Copy link
Member

@lukasz-zimnoch lukasz-zimnoch left a comment

Choose a reason for hiding this comment

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

Excellent work! Just a few comments:

typescript/src/lib/contracts/bridge.ts Outdated Show resolved Hide resolved
typescript/src/lib/bitcoin/ecdsa-key.ts Outdated Show resolved Hide resolved
typescript/src/lib/bitcoin/hash.ts Outdated Show resolved Hide resolved
typescript/src/lib/bitcoin/address.ts Show resolved Hide resolved
@tomaszslabon
Copy link
Contributor Author

tomaszslabon commented Oct 25, 2023 •
edited
Loading

Due to problems with an unverified commit and a complicated commit history we decided to close this PR in favour of #723, which contains the same changes.

auto-merge was automatically disabled October 25, 2023 11:01

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukasz-zimnoch lukasz-zimnoch lukasz-zimnoch approved these changes

Assignees

@tomaszslabon tomaszslabon

Labels
🔌 typescript TypeScript library
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tomaszslabon @lukasz-zimnoch