wallet-selector icon indicating copy to clipboard operation
wallet-selector copied to clipboard

Removed contractId and added made contracts required, signAndSendTran…

Open amirsaran3 opened this issue 1 year ago • 1 comments

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: https://github.com/near/wallet-selector/pull/811#discussion_r1242130459. 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:

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

Type of change.

  • [ ] FIX - a PR of this type patches a bug.
  • [x] 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 avatar Jul 03 '23 10:07 amirsaran3

@amirsaran3 when it will be merged?

AZbang avatar Feb 12 '24 22:02 AZbang

@amirsaran3 @kujtimprenkuSQA Can we merge this PR?

gtsonevv avatar Jul 26 '24 10:07 gtsonevv

@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 👍

kujtimprenkuSQA avatar Jul 29 '24 13:07 kujtimprenkuSQA

@kujtimprenkuSQA Perfect! Which docs exactly?

gtsonevv avatar Jul 30 '24 08:07 gtsonevv

@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

kujtimprenkuSQA avatar Jul 30 '24 08:07 kujtimprenkuSQA