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

Implement Fallback to User Balance for Transactions When Relayer Fails #223

Merged
merged 36 commits into from
Jun 11, 2024

Conversation

Pessina
Copy link
Collaborator

@Pessina Pessina commented Apr 30, 2024

This PR introduces a fallback mechanism for transactions when the relayer service fails to relay them due to specific issues such as actions not being allowed or the sender/receiver not being whitelisted. In such cases, the transaction will be retried using the user's own balance, ensuring that the transaction can still be processed without dependency on the relayer's current state or configuration.

Copy link

vercel bot commented Apr 30, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
fast-auth-signer ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 11, 2024 9:55am

@Pessina Pessina requested review from benmalcom and hcho112 and removed request for benmalcom May 1, 2024 06:06
@hcho112
Copy link
Collaborator

hcho112 commented May 2, 2024

While I was looking at the code, it comes to my mind that, would it be nicer if:

  1. We inform the user (or developer of this code or both) that upon relayer fail, we will attempt to send transaction with user balance.
  2. Toggle switch logic (either via UI or config via developer) where user agrees to execute the transaction via their account balance.

TDLR, I don't think just assume that user wants to transaction with their account balance is a good thing. It is possible that either they may not want this feature nor not being aware of this will happen. (Some user may even think this is a scam because they may expect using fast-auth + relayer would pay the gas but it ends up using user's balance)

Ideally, we should implement both where:

  1. upon configuration/implementation, if we pass allowToUseUserBalance or some sort of boolean flag.
  2. If above boolean flag is set to true and if it is multi-chain, we show small UI boolean flag with description about "Allow to use user balance to send transaction".
  3. If signTransaction is called and relayer is failed, we inform user in some way? (show UI/modal/popup/notification etc)

and need to rebase and fix yarn lock file

@Pessina
Copy link
Collaborator Author

Pessina commented May 2, 2024

@hcho112 Good point!

For the developer part, I agree with you, and it would be beneficial to allow developers to configure whether they want users to pay for the transaction in case the relayer refuses the transaction. We can bring this up for discussion. The issue here is that we would need to change the wallet interface. From what I understand, this requires a NEP, right? I propose we handle this later as a separate ticket.

However, for the user part, I believe they are already aware of the fees as it's already stated on the iFrame of the fast-auth-signer.

Screenshot 2024-05-02 at 2 40 53 PM

As you can see, the total already includes the fees. Users assume they will pay the gas, but sometimes they won't. It's a positive surprise in that case.

What do you think?

@Pessina
Copy link
Collaborator Author

Pessina commented May 2, 2024

@hcho112 regarding the multi-chain, this PR doesn't touched multi-chain code, it's only for the /sign method.

@hcho112
Copy link
Collaborator

hcho112 commented May 3, 2024

@Pessina
Personally I think this is a good change, I would rather have something that works even if I need to pay transaction with my account. However I would rather confirm with @diogoortega and @nall-near for further feedback. Can we get your feedback?

In terms of making change on near-fastauth-wallet, I don't think it needs to be mentioned in NEP. We can make the config changes by updating FastAuthWalletParams type with adding extra prop and pass down from there?

@diogoortega
Copy link
Contributor

Agree that it's great to have a fallback mechanism instead of just having the transaction fail.
However, we should inform the user. When will we know if the transaction will be "sponsored" (via relayer) or not? Can we show that info in the confirmation dialog linked above or would we only know after the transaction is confirmed by the user?

If we don't know beforehand, maybe we could add a checkbox (default ON) that says something like "Use my balance for this transaction if sponsoring is not available". Similar to this toggle on iOS that tries to send your text via SMS when iMessage isn't available:

IMG_9836

What do you think @nall-near?

@nall-near
Copy link

I think the context of why the transaction fails is important. For example, in the case of un-allowed actions, or non-whitelisted receiver ID, these hurdles could be intentional to protect the user from carelessly signing transactions.

Similar to Dio's question above,

When will we know if the transaction will be "sponsored" (via relayer) or not?
Will we know that the proposed transaction does not meet the appropriate criteria (whitelist, allowed, etc.) before signing?

Regardless, I agree with @diogoortega that we need to inform the user if something is different/unexpected about signing, and try to be as explicit as possible.
A. Txn fails because no more gas allotted | Inform user that they will need to pay for the fees
B. Txn fails because the receiver is not whitelisted | Inform the user this is not inherently trusted and they should proceed with caution
C. Txn fails because the action is not allowed | similar to B., inform the user that this action is not automatic and to confirm before continuing

Let me know if I'm overthinking this, we don't need unique messaging for each use case, but I do want to alert the user with some caution if they are trying to do something outside the allowed context of the relayer as well as when they may need to pay for the fees themselves.

I also like Dio's suggestion to have a toggle within the dapp context settings that allows users to confirm automatically.

@nall-near
Copy link

We could use something like this for the signer app, it could also be a little less scary if that makes sense to y'all.
image

@Pessina
Copy link
Collaborator Author

Pessina commented May 8, 2024

@diogoortega and @nall-near, thank you for all the input. Let me try to answer your questions, and please let me know if anything is unclear.

When will we know if the transaction will be "sponsored" (via relayer) or not?

We will only know if the transaction will be "sponsored" after sending it via the relayer and checking the response.

Regarding the custom error messages: yes, it's possible. The relayer returns a custom message with some clues about why it failed. I can double-check the messages so we can customize them accordingly.

Will we know that the proposed transaction does not meet the appropriate criteria (whitelist, allowed, etc.) before signing?

No, we only find out after calling the relayer.

We can set up a call with the relayer team to discuss approaches for obtaining this information before calling the send transaction endpoint on the relayer. However, I would advise doing this in a separate PR as it may take time.


  • Toggle Box: I like this approach as it's easy to implement, and we only ask for user confirmation once.
  • Custom Error: I'm not a big fan of this approach for our current implementation, as it would require prompting the user for confirmation twice:
    1. Prompt the iFrame to sign the SignDelegate action and send it to the relayer.
    2. If the relayer fails, we prompt the iFrame again with the error and ask for the user's confirmation to sign the transaction.

Note: User confirmation is only possible when the transaction requires the FullAccess Key. For the FunctionCall Key, the dApp holds the key and can sign transactions on behalf of the user without confirmation.

Currently, we do not have a UI for the FunctionCall Key. We can develop one, but the dApp would still be able to bypass this confirmation if desired.


Let me provide more details about the flow:

1 - FullAccess Key:

  • dApp sends a transaction to fast-auth-wallet.
  • fast-auth-wallet calls fast-auth-signer asking for a signature.
  • fast-auth-signer iFrame pops up, and the user clicks on Confirm to sign the SignedDelegate.
  • fast-auth-signer returns the SignedDelegate to fast-auth-wallet.
  • fast-auth-wallet sends the transaction through the relayer.
  • the transaction either succeeds or fails based on the response from the relayer.

2 - FunctionCall Key:

  • dApp sends a transaction to fast-auth-wallet.
  • fast-auth-wallet signs the transaction with the FunctionCall Key.
  • fast-auth-wallet sends the transaction through the relayer.
  • The transaction either succeeds or fails based on the response from the relayer.

The main difference is that with the FullAccess Key, we display the iFrame for user confirmation. In the second scenario, we do not have a UI to ask the user to confirm if they want to use their own balance as the dApp holds the FunctionCall Key and can sign transaction on behalf of the user without asking.

@Pessina
Copy link
Collaborator Author

Pessina commented May 8, 2024

I just had a chat with @DavidM-D about this issue, and he came up with a nice suggestion that we think may simplify the
flow.

The dApp probably knows if the transaction can or cannot be relayed, so we could expose two endpoints for the transaction signature:

  • Relayer endpoint (signs DelegateAction)
  • User balance endpoint (signs Transaction)

By doing this, we can create two distinct user interfaces to communicate the transaction details to the user, rather than having a single interface that attempts to handle both scenarios with a toggle box or by prompting the iFrame twice for confirmation.

What are your thoughts on this approach?

Also, I asked him how hard it would be to implement the feature to check if the relayer will accept/refuse the transaction before sending it. He said it will be complicated to do that, so it would be better if we avoid this path.

Copy link
Collaborator

@hcho112 hcho112 left a comment

Choose a reason for hiding this comment

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

Code looks good, wanted to test this out but we can't at the moment due to indexer issue.

Meanwhile, wonder if it is possible to have a end to end test covering the new sign and sign-transaction route?

Copy link
Collaborator

@hcho112 hcho112 left a comment

Choose a reason for hiding this comment

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

@Pessina I have tested them according to your recommendation but I couldn't get any of them working properly. Maybe perhaps we should try to test this together? meanwhile can you share with us video recording of working version if you can get this working locally?

image

image

@Pessina
Copy link
Collaborator Author

Pessina commented Jun 11, 2024

@hcho112 I believe the error you are facing it's because you didn't linked the local near-fastauth-wallet package, that's why it's not finding the methods.

In order to do that you need to run pnpm link [path to your near-fastauth-wallet build] on near-discovery root folder, then you can paste the test code I shared with you on Slack and conduct the testing.

Those are the recordings:

Screen.Recording.2024-06-11.at.11.27.01.mov
Screen.Recording.2024-06-11.at.11.24.59.mov

If you still face issues, please let me know and we can work on this together.

hcho112
hcho112 previously approved these changes Jun 11, 2024
Copy link
Collaborator

@hcho112 hcho112 left a comment

Choose a reason for hiding this comment

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

LGTM!! awesome work

Copy link
Collaborator

@esaminu esaminu left a comment

Choose a reason for hiding this comment

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

Code looks good and passing in my local testing 🎉

@Pessina Pessina merged commit 67488cc into main Jun 11, 2024
4 checks passed
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