Skip to content

Mahmoud/eng 3111 integrate with stacks raw hex signing on extension#652

Merged
m-aboelenein merged 22 commits into
developfrom
mahmoud/eng-3111-integrate-with-stacks-raw-hex-signing-on-extension
Dec 27, 2023
Merged

Mahmoud/eng 3111 integrate with stacks raw hex signing on extension#652
m-aboelenein merged 22 commits into
developfrom
mahmoud/eng-3111-integrate-with-stacks-raw-hex-signing-on-extension

Conversation

@m-aboelenein

@m-aboelenein m-aboelenein commented Nov 9, 2023

Copy link
Copy Markdown
Contributor

🔘 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

This PR introduces support for signing a transaction hex, which is parsed and displayed for the user for signing this enables stx multisig transactions

Issue Link: #ENG-3111

🔄 Changes

  • added flags to determine if the tx is multisig or not
  • disable advanced settings for confirm-stx-transaction screen if the tx already has signatures

related core pr: secretkeylabs/xverse-core#285

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

@m-aboelenein
m-aboelenein requested a review from teebszet November 9, 2023 21:21
@m-aboelenein m-aboelenein self-assigned this Nov 9, 2023
Comment thread src/app/screens/transactionRequest/index.tsx Outdated
Comment thread src/app/screens/confirmStxTransaction/index.tsx
Comment thread src/app/screens/confirmStxTransaction/index.tsx Outdated
Comment thread src/app/components/transactionsRequests/utils.ts Outdated
…ntegrate-with-stacks-raw-hex-signing-on-extension
…ntegrate-with-stacks-raw-hex-signing-on-extension
Comment thread src/app/components/transactionsRequests/ContractCallRequest.tsx Outdated
Comment thread src/app/screens/sendNft/index.tsx Outdated
teebszet
teebszet previously approved these changes Nov 24, 2023
@teebszet

Copy link
Copy Markdown
Contributor

nice work! 🙌
image

Comment thread src/app/screens/transactionRequest/index.tsx
Conflicts:
	package-lock.json
	package.json
	src/app/screens/confirmFtTransaction/index.tsx
	src/app/screens/confirmStxTransaction/index.tsx
	src/app/screens/sendFt/index.tsx
	src/app/screens/signatureRequest/index.tsx
	src/app/screens/swap/useAlexSponsoredTransaction.ts
	src/app/screens/transactionRequest/helper.ts
	src/common/types/message-types.ts
Comment thread src/inpage/sats.inpage.ts Outdated
};

const SatsMethodsProvider: BitcoinProvider = {
const SatsMethodsProvider: Partial<BitcoinProvider> = {

@teebszet teebszet Dec 4, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm curious what was the reasoning for Partial here? 😄

do we want to hide type errors when we forget to add a new method from sats-connect? 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

in my case, the version of sats-connect that we had in package.json was ahead of what was merged in develop at the time we might not want to keep it though, it's necessary now so the build doesn't fail

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ah I see. I'd be keen to remove it before merge, since it was a good code hint before

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@m-aboelenein did you have a chance to follow up on this one?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

nice catch i reverted that change 👍

import { decodeToken } from 'jsontokens';
import { useLocation } from 'react-router-dom';

const useStxTransactionRequest = () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

would be good to have a return type here. related to the core code review on txPayloadToRequest

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There's an issue with strictly typing this hook since the types that are exposed from the @stacks/connect lib have errors with the types themselves so until that's resolved loosely typing here is our only solution.

I will open an issue on the @stacks/connect repo and add a follow-up issue.

…ntegrate-with-stacks-raw-hex-signing-on-extension
teebszet
teebszet previously approved these changes Dec 5, 2023
…ntegrate-with-stacks-raw-hex-signing-on-extension
Comment thread package.json
"@react-spring/web": "^9.6.1",
"@secretkeylabs/xverse-core": "6.0.1",
"@stacks/connect": "^6.10.2",
"@stacks/encryption": "4.3.5",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks like we currently still include @stacks/encryption in webpack/webpack.config.js, so I think it would make sense to remove it from there as well

@DuskaT021 DuskaT021 added returned Tested by the QA, didn't pass checks and removed ready for test labels Dec 25, 2023
@DuskaT021

Copy link
Copy Markdown
Contributor

Will be picked up for testing, after a comment gets resolved

dhriaznov
dhriaznov previously approved these changes Dec 27, 2023
@dhriaznov dhriaznov added ready for test and removed returned Tested by the QA, didn't pass checks labels Dec 27, 2023
)

* fix: regenerate the unsignedTx with correct stxAddress after switch account

* fix: minor cleanup

* fix tx-request account switching

* updated core version

* added type checking and default values for feeMultipliers

---------

Co-authored-by: Mahmoud Aboelenein <mahmoud@secretkeylabs.com>
@github-actions

Copy link
Copy Markdown

@DuskaT021 DuskaT021 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Lgtm

@m-aboelenein
m-aboelenein merged commit 65d33e3 into develop Dec 27, 2023
@m-aboelenein
m-aboelenein deleted the mahmoud/eng-3111-integrate-with-stacks-raw-hex-signing-on-extension branch December 27, 2023 17:52
This was referenced Dec 29, 2023
fedeerbes pushed a commit that referenced this pull request Oct 4, 2024
…functionality-from-ext

[ENG-5340] Export necessary functionality from the extension to core
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants