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: updated ordinals tx functions #552

Conversation

abdulhaseeb4239
Copy link
Contributor

@abdulhaseeb4239 abdulhaseeb4239 commented Aug 8, 2023

🔘 PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Enhancement
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no API changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

📜 Background

Moved ordinal transactions logic to xverse-core

Issue Link: https://linear.app/xverseapp/issue/ENG-2551/updated-functions-for-ordinal-transactions
Context Link (if applicable): https://linear.app/xverseapp/issue/ENG-2528/move-ordinal-transactions-logic-to-xverse-core

🔄 Changes

Updated ordinals transactions functions from xverse-core
This PR depends on xverse-core PR secretkeylabs/xverse-core#218

Impact:

  • Send Ordinal, Restore Ordinal

Testing:
I have tested send Ordinal and recover Ordinal flow on iOS simulator.

Testing Instruction:

  • Make sure Send Ordinal from collectibles is working fine including the fee calculation edit
  • Make sure Recover Ordinals flow from settings is working fine including the fee calculation and fee edit

🖼 Screenshot / 📹 Video

Screen.Recording.2023-08-15.at.12.13.10.PM.mov

✅ 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.

@teebszet
Copy link
Member

teebszet commented Aug 11, 2023

@abdulhaseeb4239 code looks good, but missing videos or details of local tests.

@DuskaT021
Copy link
Contributor

has conflicts

@abdulhaseeb4239 abdulhaseeb4239 self-assigned this Aug 14, 2023
…-extension into abdulhaseeb/eng-2551-updated-functions-for-ordinal-transactions
@teebszet
Copy link
Member

teebszet commented Aug 17, 2023

manually tested. send ordinal flow looks good, but recover ordinal looks broken:

image

teebszet
teebszet previously approved these changes Aug 17, 2023
@teebszet teebszet self-requested a review August 17, 2023 04:40
@teebszet teebszet dismissed their stale review August 17, 2023 04:43

restore ordinal flow, see comment

teebszet
teebszet previously approved these changes Aug 21, 2023
@teebszet
Copy link
Member

@abdulhaseeb4239 the recover flow wasn't working? pr has conflicts again

…-extension into abdulhaseeb/eng-2551-updated-functions-for-ordinal-transactions
@abdulhaseeb4239
Copy link
Contributor Author

@abdulhaseeb4239 the recover flow wasn't working? pr has conflicts again

yes, it was not working properly. the ordinal UTXOs were not being fetched from API to filter UTXOs from fees. The change is in core

teebszet
teebszet previously approved these changes Aug 24, 2023
@DuskaT021
Copy link
Contributor

I'll be testing this pr with the latest xverse-core https://github.com/secretkeylabs/xverse-core/releases/latest at the time of writing it was 1.6.5

@DuskaT021
Copy link
Contributor

left my findings in the linear ticket ENG-2551

@DuskaT021
Copy link
Contributor

manually tested. send ordinal flow looks good, but recover ordinal looks broken:

image

it is still broken

@DuskaT021 DuskaT021 added returned Tested by the QA, didn't pass checks and removed ready for test labels Aug 29, 2023
…-extension into abdulhaseeb/eng-2551-updated-functions-for-ordinal-transactions
teebszet
teebszet previously approved these changes Sep 1, 2023
@teebszet teebszet added ready for test and removed returned Tested by the QA, didn't pass checks labels Sep 1, 2023
@teebszet
Copy link
Member

teebszet commented Sep 5, 2023

@DuskaT021 was this one ready to merge? I can't remember if it was on a discord thread

@DuskaT021
Copy link
Contributor

@DuskaT021 was this one ready to merge? I can't remember if it was on a discord thread

it was in linear here - not ready for merge, needs to be retested, after the xverse-core updated

@DuskaT021
Copy link
Contributor

@abdulhaseeb4239 needs conflicts resolved

@DuskaT021 DuskaT021 self-assigned this Sep 13, 2023
@abdulhaseeb4239
Copy link
Contributor Author

@abdulhaseeb4239 needs conflicts resolved

done

@DuskaT021
Copy link
Contributor

left a comment in the linear ticket @abdulhaseeb4239

@DuskaT021 DuskaT021 added returned Tested by the QA, didn't pass checks and removed ready for test labels Sep 14, 2023
@github-actions
Copy link

@yknl yknl closed this Sep 18, 2023
@teebszet teebszet deleted the abdulhaseeb/eng-2551-updated-functions-for-ordinal-transactions branch November 23, 2023 07:11
fedeerbes pushed a commit that referenced this pull request Oct 4, 2024
…ering

Remove Alex SIP-10 tokens filtering
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
returned Tested by the QA, didn't pass checks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants