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

Refactor transaction logic for ease of use #346

Merged
merged 23 commits into from
Feb 15, 2024
Merged

Conversation

victorkirov
Copy link
Member

🔘 PR Type

What kind of change does this PR introduce?

  • Enhancement

📜 Background

Minor changes to make the consolidated logic nicer to consume

🔄 Changes

Does this PR introduce a breaking change?

  • Yes, Incompatible API changes
  • No, Adds functionality (backwards compatible)
  • No, Bug fixes (backwards compatible)

Changes:

  • Mostly new types added and some renaming

Impact:

  • Nothing is using this logic as yet, so no changes in extension/mobile needed

✅ Review checklist

Please ensure the following are true before merging:

  • Code Style is consistent with the project guidelines.
  • Code is readable and well-commented.
  • No unnecessary or debugging code has been added.
  • Security considerations have been taken into account.
  • The change has been manually tested and works as expected.
  • Breaking changes and their impacts have been considered and documented.
  • Code does not introduce new technical debt or issues.

@victorkirov victorkirov self-assigned this Jan 10, 2024
transactions/bitcoin/context.ts Outdated Show resolved Hide resolved
@teebszet
Copy link
Member

teebszet commented Feb 1, 2024

I wonder if it's worth effort to make this PR backwards compatible? I remember our discussion with @fedeerbes around reducing breaking changes for when mobile is forced to bump core for certain functionality, but then required to refactor/fix for breakages

@victorkirov
Copy link
Member Author

I wonder if it's worth effort to make this PR backwards compatible? I remember our discussion with @fedeerbes around reducing breaking changes for when mobile is forced to bump core for certain functionality, but then required to refactor/fix for breakages

This is backwards compatible, purely from the fact that this code is not used in mobile yet 😋

@victorkirov
Copy link
Member Author

@m-aboelenein @teebszet ready for final review 🙏

@teebszet
Copy link
Member

teebszet commented Feb 2, 2024

This is backwards compatible, purely from the fact that this code is not used in mobile yet 😋

cool. still technically a major version bump right?

@teebszet teebszet added the major label Feb 2, 2024
@victorkirov
Copy link
Member Author

This is backwards compatible, purely from the fact that this code is not used in mobile yet 😋

cool. still technically a major version bump right?

Yup, there are some interface changes on things that weren't used before.

Copy link
Contributor

github-actions bot commented Feb 6, 2024

Test this PR with npm i --save-exact @secretkeylabs/[email protected]

@teebszet teebszet merged commit 2d298d2 into develop Feb 15, 2024
3 checks passed
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.

3 participants