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

Fix: .signTransaction from dapp browser web3 call should only sign not sign+send #6357

Conversation

hboon
Copy link
Member

@hboon hboon commented Feb 17, 2023

Fixes #6351

@hboon hboon force-pushed the fix-signtransaction-from-dapp-browser-web3-call-should-only-sign-not-sign+send branch from 8f63d5d to d5f11cd Compare February 17, 2023 01:49
@hboon hboon requested a review from oa-s February 20, 2023 03:11
oa-s
oa-s previously approved these changes Feb 20, 2023
Copy link
Contributor

@oa-s oa-s left a comment

Choose a reason for hiding this comment

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

uniswap looks broken with this pr

@@ -499,7 +499,7 @@ extension DappBrowserCoordinator: BrowserViewControllerDelegate {
func performDappAction(account: AlphaWallet.Address) {
switch action {
case .signTransaction(let unconfirmedTransaction):
executeTransaction(action: action, callbackID: callbackID, transaction: unconfirmedTransaction, type: .signThenSend)
executeTransaction(action: action, callbackID: callbackID, transaction: unconfirmedTransaction, type: .sign)
Copy link
Contributor

@oa-s oa-s Feb 17, 2023

Choose a reason for hiding this comment

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

@hboon It looks like not all changes that is needed, guess there is mistake in js file aplied to WebView.
we handle processTransaction from js file, and treat it as signTransaction that we receive in web browser, out of js file
HookedWalletSubprovider.prototype.processTransaction uses for sending transaction.
there is also processSignTransaction, that should be used for signing transaction, and looks like we skipping it
HookedWalletSubprovider.prototype.processSignTransaction

Copy link
Contributor

Choose a reason for hiding this comment

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

we might need to add webViewConfig.userContentController.add(messageHandler, name: Method.sendTransaction.rawValue) as there is no such line in addition there might be need to replace processTransaction to make it send sendTransaction insteead of signTransaction:

                   processTransaction: function (tx, cb){
                       console.log('sending a transaction', tx)
                       const { id = 8888 } = tx
                       AlphaWallet.addCallback(id, cb)
                       webkit.messageHandlers.sendTransaction.postMessage({"name": "sendTransaction", "object":     tx, id: id})
                   },

Copy link
Contributor

Choose a reason for hiding this comment

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

and also need to define processSignTransaction for AlphaWallet js provider, looks like it not visible, the is required js file changes

Copy link
Contributor

Choose a reason for hiding this comment

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

@hboon do we have example dapp for testing, looks like uniswap don't work with goerli anymore, (at list for me)

Copy link
Member Author

Choose a reason for hiding this comment

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

Not that I know of now. I can dig into this issue/PR if you like. Might build a test dapp.

Copy link
Member Author

Choose a reason for hiding this comment

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

When testing against Uniswap, I use Polygon — cheaper :P

@hboon
Copy link
Member Author

hboon commented Feb 20, 2023

Can you help reject the PR first? Just in case. Clearer :)

@oa-s oa-s self-requested a review February 20, 2023 09:58
@oa-s oa-s dismissed their stale review February 20, 2023 10:02

rejection

@oa-s
Copy link
Contributor

oa-s commented Feb 20, 2023

Can you help reject the PR first? Just in case. Clearer :)

am i did right?

@hboon
Copy link
Member Author

hboon commented Feb 20, 2023

Seems ok. GitHub seems a little strange. Not what I expected. But looks alright. Thanks.

@hboon
Copy link
Member Author

hboon commented Feb 21, 2023

Apparently we don't support eth_signTransaction (which is sign, but not send) in the dapp browser. We only support eth_sendTransaction (which is sign+send). Created AlphaWallet/AlphaWallet-web3-provider#36. Will delay it unless it's necessary for Uniswap.

(maybe you were referring to tat missing support earlier up there)

Can you help to verify that it Uniswap works with master (if it does, can you list the steps to reproduce?). If it does and this PR breaks it, then we'll fix it, otherwise, I'll clean this PR up and try to merge it again

@hboon
Copy link
Member Author

hboon commented Feb 21, 2023

Oh I checked the PR, and looks like if we rebase master, it already includes the changes in this PR, i.e. .signThenSend -> .sign for .signTransaction since the handling for signing and sending transaction are now centralized with the recent refactor.

So we can close this PR once we figure out if Uniswap is broken for us.

@oa-s
Copy link
Contributor

oa-s commented Mar 1, 2023

@hboon looks like this can be closed, isn't it?

@hboon
Copy link
Member Author

hboon commented Mar 1, 2023

Yes. Seems so. I’ll take a look to be sure.

@hboon hboon closed this Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.signTransaction from dapp browser should only sign, not sign + send
2 participants