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

Move vuex hdWallet methods to SDK wallet lib #2409

Merged

Conversation

peronczyk
Copy link
Collaborator

No description provided.

@peronczyk peronczyk force-pushed the refactor/move-vuex-hdwallet-methods-to-sdk-wallet-lib branch 2 times, most recently from 46c8d5e to ac7abc6 Compare October 18, 2023 13:02
@peronczyk peronczyk force-pushed the refactor/move-permissions-vuex-state-to-composable branch 2 times, most recently from 5853684 to 917b518 Compare October 20, 2023 06:25
@peronczyk peronczyk force-pushed the refactor/move-vuex-hdwallet-methods-to-sdk-wallet-lib branch from ac7abc6 to 1e9d4ec Compare October 20, 2023 06:26
@peronczyk peronczyk force-pushed the refactor/move-permissions-vuex-state-to-composable branch from 917b518 to 358a241 Compare October 23, 2023 07:15
@peronczyk peronczyk force-pushed the refactor/move-vuex-hdwallet-methods-to-sdk-wallet-lib branch from 1e9d4ec to d445277 Compare October 23, 2023 07:19
@peronczyk peronczyk force-pushed the refactor/move-permissions-vuex-state-to-composable branch 2 times, most recently from df4c5e8 to 53cae92 Compare October 23, 2023 10:43
Base automatically changed from refactor/move-permissions-vuex-state-to-composable to develop October 23, 2023 11:12
@peronczyk peronczyk force-pushed the refactor/move-vuex-hdwallet-methods-to-sdk-wallet-lib branch from d445277 to 21e1e69 Compare October 23, 2023 11:22
@peronczyk peronczyk force-pushed the refactor/move-vuex-hdwallet-methods-to-sdk-wallet-lib branch 4 times, most recently from ae128c4 to d2f6c8b Compare October 24, 2023 20:39
@github-actions
Copy link

@peronczyk peronczyk marked this pull request as ready for review October 24, 2023 20:58
@peronczyk peronczyk changed the title [DRAFT] Move vuex hdWallet methods to SDK wallet lib Move vuex hdWallet methods to SDK wallet lib Oct 24, 2023
@peronczyk peronczyk linked an issue Oct 24, 2023 that may be closed by this pull request
Comment on lines +68 to +70
message: params.message,
tx: params.tx,
txBase64: params.txBase64,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Previously the Raw Sign modal was receiving the props in a different format than the regular sign modal. I aligned both modals as they both are using the PopupProps composable. Now the structure is enforced by the TypeScript - less possible to make a mistake.

@peronczyk peronczyk force-pushed the refactor/move-vuex-hdwallet-methods-to-sdk-wallet-lib branch 3 times, most recently from 1c1631d to 21b67d8 Compare October 25, 2023 07:50
Comment on lines +170 to +176
if (
method === METHODS.sign
&& (!modalProps.txBase64 || !isTxOfASupportedType(modalProps.txBase64))
) {
modal = MODAL_CONFIRM_RAW_SIGN;
popup = POPUP_TYPE_RAW_SIGN;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Previously the openPopup was detecting if the transaction is supported by the app or not. The same check was done in the previous permissions Vuex module. So there was 2 places doing the same logic. I consolidated this and moved here as the permissions composable is the only place where the openPopup function is fired.

? popupProps.value?.data
: Buffer.from(popupProps.value?.data as any).toString('hex'),
);
const dataAsString = computed((): string => popupProps.value?.txBase64?.toString() || '');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As there is no such a thing as data anymore we were able to skip any conditional checks.

Copy link
Collaborator Author

@peronczyk peronczyk Oct 25, 2023

Choose a reason for hiding this comment

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

This is the most important file of the PR. It consists all the logic we had in the Vuex hdWallet module. So this is a core of signing stuff and asking user for permission to do it.
Following methods does not exist anymore:

  1. signTransactionWithoutConfirmation - calculating if the confirmation modal should be open is now done in a different way.
  2. signTransactionFromAccount - passing fromAccount as one of the properties of any of the default sign method options causes to sign with the specified account.

Copy link
Member

@davidyuk davidyuk Oct 27, 2023

Choose a reason for hiding this comment

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

This class looks good 👍

@peronczyk peronczyk force-pushed the refactor/move-vuex-hdwallet-methods-to-sdk-wallet-lib branch 3 times, most recently from cadb511 to f0f4087 Compare October 26, 2023 09:00
@Liubov-crypto
Copy link
Collaborator

During my testing I've constantly got this issue in console, and I think it might be affect all errors that I listed below:

Error: pubkey cannot be null or undefined.

err1

  1. Can't send ae and Fungible tokens:
  2. Verify an amount and Max button isn't working for FT:
2023-10-26.4.40.34.mov
  1. Set name pointer:
    set pointer

  2. Register a name:
    reg

  3. Multisig creation

  4. Generate Gift card

  5. Bid on name

  6. Transactions on DEX:
    swap

  7. Interactions with superhero.com - I can connect my wallet but there is no popup for signing Txs.

@peronczyk please check it.

@peronczyk peronczyk force-pushed the refactor/move-vuex-hdwallet-methods-to-sdk-wallet-lib branch 2 times, most recently from 1a2d0e8 to 8ca035f Compare October 31, 2023 07:49
@peronczyk
Copy link
Collaborator Author

@Liubov-crypto thank you for your comments. I addressed your feedback. Please retest.

@peronczyk peronczyk force-pushed the refactor/move-vuex-hdwallet-methods-to-sdk-wallet-lib branch 3 times, most recently from 84a1f80 to a4882aa Compare November 2, 2023 12:09
@peronczyk peronczyk self-assigned this Nov 3, 2023
@peronczyk peronczyk force-pushed the refactor/move-vuex-hdwallet-methods-to-sdk-wallet-lib branch from a4882aa to fc4876a Compare November 6, 2023 10:43
@peronczyk peronczyk force-pushed the refactor/move-vuex-hdwallet-methods-to-sdk-wallet-lib branch from fc4876a to 813a18a Compare November 6, 2023 13:51
Copy link
Contributor

@martinkaintas martinkaintas left a comment

Choose a reason for hiding this comment

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

lgtm

@peronczyk peronczyk force-pushed the refactor/move-vuex-hdwallet-methods-to-sdk-wallet-lib branch from 813a18a to 5b6f1be Compare November 6, 2023 13:53
Copy link
Collaborator

@CedrikNikita CedrikNikita left a comment

Choose a reason for hiding this comment

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

Code changes look reasonable.

@subhod-i
Copy link
Contributor

subhod-i commented Nov 9, 2023

Code changes looks good to me. Apart of that please fix the bugs we discussed.

@peronczyk peronczyk force-pushed the refactor/move-vuex-hdwallet-methods-to-sdk-wallet-lib branch from 5b6f1be to c72d3d7 Compare November 10, 2023 07:48
@peronczyk peronczyk force-pushed the refactor/move-vuex-hdwallet-methods-to-sdk-wallet-lib branch from c72d3d7 to 077e957 Compare November 10, 2023 08:16
Copy link
Contributor

@subhod-i subhod-i left a comment

Choose a reason for hiding this comment

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

Cool PR.

@peronczyk peronczyk merged commit 7c3cb65 into develop Nov 10, 2023
4 checks passed
@peronczyk peronczyk deleted the refactor/move-vuex-hdwallet-methods-to-sdk-wallet-lib branch November 10, 2023 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move the account signing methods from Vuex to composable
6 participants