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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion AlphaWallet/Browser/Coordinators/DappBrowserCoordinator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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

case .sendTransaction(let unconfirmedTransaction):
executeTransaction(action: action, callbackID: callbackID, transaction: unconfirmedTransaction, type: .signThenSend)
case .signMessage(let hexMessage):
Expand Down