-
Notifications
You must be signed in to change notification settings - Fork 158
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
feat: Add signInMulti
method
#811
base: dev
Are you sure you want to change the base?
Conversation
signInMulti
methodsignInMulti
method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comments around keeping state for signInMulti()
separate from signIn()
-- we need to make sure that a combination of signIn()
and signInMulti()
being called with the same contracts will result in sane and deterministic behaviour.
return accounts; | ||
}; | ||
|
||
wallet.signInMulti = async (params: never) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than having every wallet module be required by convention to implement a signInMulti()
method that throw
's an error if it isn't supported, can we instead not add a signInMulti()
method in the modules that do not support it, and make this decorator check if the method exists and throw the error
throw new Error(
`The signInMulti method is not supported by ${metadata.name}`
);
if it hasn't been implemented?
This has two benefits:
- If any wallet module doesn't implement the method for whatever reason, callers will always get a consistent error thrown instead of there being a '_signInMulti() is not a function' type of error thrown from our attempt to call it in those cases
- We don't need to inform wallet module authors about the need to add a stub, ensure their modules have the stub during code reviews, etc. -- we can treat the lack of implementation as a 'not supported'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed it in this commit: 8121e5e
packages/core/src/lib/store.types.ts
Outdated
/** | ||
* The list of contracts when SignInMulti is supported. | ||
*/ | ||
contracts: MultiContractState | null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to handle cases where:
- A dapp might call
signIn()
for a contract that was previously signed into usingsignInMulti()
. For example, let's say that a dapp uses 3 contracts regularly, and their access key allowance expires for just one of those three contracts. The dapp author does not need to request the user re-signIn to all 3 contracts usingsignInMulti()
-- they would just requestsignIn()
for the one contract whose allowance has been used up. - A dapp might call
signInMulti()
with an array that includes a contract that they had previously signed in to usingsignIn()
If we always store results from signInMulti()
in contracts
and results from signIn()
in contract
, we will end up with duplicate entries in these keys when a mixture of calls is used by the dapp developer -- we need to have one-and-only-one entry for a given contract across these two fields (contract
and contracts
) to make sure everything is sane and deterministic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR should solve these issues: #847
…ting wallet if it does not exist.
f910c39
…saction now requires receiverId
Hi @kujtimprenkuSQA @MaximusHaximus What is the status of this PR? Our team is recently tried to add a relevant feature and discovered this. WIP branch. I wonder if we can move this forward |
Hi, this PR is not ready to be merged yet as we still need this additional PR #847 related to the comment #811 (comment) to be reviewed and make a decision if we should go with it or not since it will introduce breaking changes in Wallet Selector. As of right now, we are not aware if any wallet supports the |
OK, so maybe I may move forward with my experiments in here then: #887 ? This approach does not require change in wallets, and does not introduce any breaking change to the wallet selector. It simply offers the option to add a function access key to another contract, and store it in a separate keystore ( near-api-js supports multiple keystore with different prefixes ). As you can see from the test case there's a new method For the particular issue for NEAR DevHub the idea is then to create the access key for the devhub contract when you submit something for the first time, and then reuse that access keys for later calls. This way, the user could also authenticate with other contracts on demand. |
See a proof of concept demo video here ( for the wallet-selector guestbook ): |
@petersalomonsen the demo is very cool! I wonder how can we invoke |
@ailisp thanks, and actually you don't need to invoke that from the DevHub widget. This should be handled by the nearsocial viewer ( or maybe VM ) so that it detects if there's a wallet redirection, and then offers to add a function access key for that contract. Here's an example of how I've added this to the viewer for my PoC with the DevHub widget: https://github.com/NearSocial/viewer/pull/189/files And this is also what's used in the demo you can see here: https://youtu.be/DVoZK5CJ2uE |
What is the plan for this feature moving forward? Any timeline estimates or prediction of when it could be available? |
The initial work for adding the We still need a review from Pagoda and further discuss this implementation before it's merged. As mentioned in the above comments the currently available keystores will have to catch up in order to support multiple keys per account near/near-api-js#1153 there was some work done related to it: near/near-api-js#1163 |
…change-fix fix near-mobile-wallet, near-snap and ramper-wallet methods
…hange Removed contractId and added made contracts required, signAndSendTran…
Description
This PR is about adding the
signInMulti
method to the public API of the wallets in Wallet Selector based on near/NEPs#428 .signInMulti
.signInMulti
in each wallet, it initially throws when calling this method.core
package to allow sign-in with multiple contracts.modal-ui
andmodal-ui-js
to pass the list ofcontracts
to thesignInMulti
method.signInMulti
.Wallets that support
signInMulti
in this PRChecklist:
Type of change.