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

Removed contractId and added made contracts required, signAndSendTran… #847

Merged

Conversation

amirsaran3
Copy link
Collaborator

@amirsaran3 amirsaran3 commented Jul 3, 2023

Description

This PR is an extension of #811. Addressing comments of Daryl on mentioned PR.

Breaking changes:

  • Removed contractId from setupModal params
  • contracts param in setupModal is now required
  • receiverId param in signAndSendTransaction is now required since we can't assume a default contract since we can have multiple

TODO:

  • Since none of the wallets support the signInMulti method right now we can't test it correctly. I am not sure how will the pending state act after these changes (for browser wallets).

Questions:

  • signInMulti method with one contract in contracts param list is same as calling signIn method? Any notable differences?
  • Regarding this comment: feat: Add signInMulti method #811 (comment). If user first signs in using two contracts and then one of them expires. He then only wants to re-sign in to the expired one and the other one is still signed in. How will the UI flow work for this interaction? Since the user is signed in they will not see a prompt to sign in again.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

Type of change.

  • FIX - a PR of this type patches a bug.
  • FEATURE - a PR of this type introduces a new feature.
  • BUILD - a PR of this type introduces build changes.
  • CI - a PR of this type introduces CI changes.
  • DOCS - a PR of this type introduces DOCS improvement.
  • STYLE - a PR of this type introduces style changes.
  • REFACTOR - a PR of this type introduces refactoring.
  • PERFORMANCE - a PR of this type introduces performance changes.
  • TEST - a PR of this type adds more tests.
  • CHORE - a PR introduces other changes than the specified above.

@amirsaran3 amirsaran3 requested a review from MaximusHaximus July 3, 2023 10:13
@amirsaran3 amirsaran3 self-assigned this Jul 3, 2023
@amirsaran3 amirsaran3 mentioned this pull request Jul 5, 2023
14 tasks
@AZbang
Copy link
Contributor

AZbang commented Feb 12, 2024

@amirsaran3 when it will be merged?

gtsonevv and others added 2 commits July 25, 2024 13:00
…change-fix

fix near-mobile-wallet, near-snap and ramper-wallet methods
@gtsonevv
Copy link
Collaborator

@amirsaran3 @kujtimprenkuSQA Can we merge this PR?

Copy link
Contributor

@kujtimprenkuSQA kujtimprenkuSQA left a comment

Choose a reason for hiding this comment

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

Hi @gtsonevv, my comments are mostly around the TS type for the contracts in the core package I left a detailed comment to explain the reason for this suggestion in the store.ts file.

If you don't think this is a necessary change you might ignore them 👍 , but if you do the change then you need to update the checks in signAndSendTransaction(s)

// From this
// Note: this is just an example.
if (!_state.wallet.isSignedIn() || contracts.length < 1) {
  throw new Error("Wallet not signed in");
}
      
// To something like this:
if (!_state.wallet.isSignedIn() || !contracts) {
  throw new Error("Wallet not signed in");
}

The docs / readme.md in modal-ui and modal-ui-js must be updated to reflect the changes of this MR.

packages/core/src/lib/store.ts Show resolved Hide resolved
packages/core/src/lib/store.types.ts Show resolved Hide resolved
packages/core/src/lib/store.types.ts Show resolved Hide resolved
packages/core/docs/api/state.md Show resolved Hide resolved
packages/here-wallet/src/lib/selector.ts Outdated Show resolved Hide resolved
packages/core/src/lib/store.ts Show resolved Hide resolved
@gtsonevv gtsonevv requested a review from kujtimprenkuSQA July 29, 2024 10:01
@kujtimprenkuSQA
Copy link
Contributor

@kujtimprenkuSQA Can we merge this PR?

@gtsonevv the PR looks good to me, I think the docs should be updated because this PR introduces breaking changes so the docs will be outdated.

I only reviewed the code which looks good to me, but the decision to merge it needs to be made by your team.

You can mark my previous comments as resolved 👍

@gtsonevv
Copy link
Collaborator

@kujtimprenkuSQA Perfect! Which docs exactly?

@kujtimprenkuSQA
Copy link
Contributor

@kujtimprenkuSQA Perfect! Which docs exactly?

The code example and the Options in this file (there's no contract anymore only contracts) refer to the code changes for better understanding:
https://github.com/near/wallet-selector/tree/SQC-540/add-signin-multi-breaking-change/packages/modal-ui

The code example and the Options on this file:
https://github.com/near/wallet-selector/tree/SQC-540/add-signin-multi-breaking-change/packages/modal-ui-js

This PR removes the .contract from the selector state:
https://github.com/near/wallet-selector/blob/SQC-540/add-signin-multi-breaking-change/packages/core/docs/api/state.md#contract

In signAndSendTransaction the receiverId is now required (remove the ?):
https://github.com/near/wallet-selector/blob/SQC-540/add-signin-multi-breaking-change/packages/core/docs/api/wallet.md#signandsendtransactionparams

@gtsonevv gtsonevv requested review from pivanov and removed request for MaximusHaximus July 30, 2024 13:14
Copy link
Contributor

@pivanov pivanov left a comment

Choose a reason for hiding this comment

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

r+

@gtsonevv gtsonevv merged commit ff6d042 into SQC-540/add-signin-multi Aug 2, 2024
2 checks passed
@gtsonevv gtsonevv deleted the SQC-540/add-signin-multi-breaking-change branch August 2, 2024 07:26
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.

5 participants