Skip to content

Conversation

@yann300
Copy link
Contributor

@yann300 yann300 commented Jul 28, 2020

This add the ability to reference new web3 provider.
Each "provider" plugin will be listed in the environment section of the run tab.

fix #30

Current state:

  • with metamask, rejecting a transaction doesn't resolve the call in the client.

  • sometimes gasEstimate returns a JSON RPC error (probably when the transaction fails), then force sending the transaction doesn't pop up anything in the mobile side.
    EDIT: https://github.com/ethereum/remix-project/pull/178/files#diff-b41d6048d9047202a90628384dd7d690R181

  • can take more than 10 seconds for a call to resolve.
    we'll probably need if the problem persist to have our own bridge.

  • call to a plugin can't be done asynchronously, so when a call doesn't resolve, all the other call to web3 are lined up (This probably can't be solved right now). this can't be solved. Plugin calls are synchronous (cc @GrandSchtroumpf fyi)

@yann300 yann300 force-pushed the addPluginProvider branch from c511a92 to 3d97156 Compare July 28, 2020 12:04
@yann300 yann300 force-pushed the addPluginProvider branch 2 times, most recently from c28b7ac to 69e17a5 Compare August 18, 2020 07:40
@yann300 yann300 force-pushed the addPluginProvider branch 5 times, most recently from b3a5907 to e20ddb8 Compare August 24, 2020 15:29
addNetwork (customNetwork) {
this.blockchain.addProvider(customNetwork)
addNetwork (network) { // { name, url }
if (network.url === 'ipc') {
Copy link
Contributor

@iurimatias iurimatias Aug 25, 2020

Choose a reason for hiding this comment

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

suggestion:

const provider = network.url === 'ipc' ? new Web3.providers.IpcProvider() : new Web3.providers.HttpProvider(network.url)
this.blockchain.addProvider({ name: network.name, provider })

}
}
this.blockchain.addProvider({ name: profile.displayName, provider: web3Provider })
})(profile, this)
Copy link
Contributor

Choose a reason for hiding this comment

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

is this really necessary (() => {})() it feels like it could be removed, and use this instead of app

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this can be removed yes.
(() => {})() is to make sure the creation of the provider is encapsulated. (that the local var profile doesn't get overridden in case the event callback is triggered multiple time.
But there might be better way to handle that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually I had to keep this ...

this.on('manager', 'pluginDeactivated', profile => {
if (profile.kind === 'provider') this.blockchain.removeProvider(profile.name)
})
this.on('manager', 'pluginActivated', profile => {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: this.on('manager', 'pluginActivated', this.addProfileProvider.bind(this) and then move the code to that function

if (profile.kind === 'provider') {
((profile, app) => {
const web3Provider = {
sendAsync (payload, callback) {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion, something like:

async sendAsync (payload, callback) {
              console.log('wallet connect', payload)
              try {
                const result = await app.call(profile.name, 'sendAsync', payload)
                console.log('wallet connect', result)
                callback(null, result)
              } catch (e) {
                  console.log('wallet connect', e)
                  callback(e)
                })
}

@yann300 yann300 force-pushed the addPluginProvider branch 2 times, most recently from a526adf to eaa811e Compare August 26, 2020 07:44
@yann300 yann300 force-pushed the addPluginProvider branch from eaa811e to bad90e2 Compare August 26, 2020 09:02
@yann300
Copy link
Contributor Author

yann300 commented Aug 26, 2020

@iurimatias just pushed some updates

@yann300 yann300 force-pushed the addPluginProvider branch from 14d39dd to 4e7491e Compare August 27, 2020 10:27
@yann300 yann300 force-pushed the addPluginProvider branch from 4e7491e to 69b9196 Compare August 27, 2020 10:58
@yann300 yann300 removed the WIP label Aug 31, 2020
@yann300 yann300 requested a review from iurimatias August 31, 2020 19:47
@yann300 yann300 mentioned this pull request Sep 3, 2020
5 tasks
@yann300 yann300 requested a review from ryestew September 10, 2020 12:48
@ryestew
Copy link
Collaborator

ryestew commented Sep 11, 2020

I like the update where you can have the walletconnect plugin as the active tab and the txn can still be run. The Clear Current Request button - is important - we'll see if users can find it - but if they find the button ( by realizing they should look at the walletconnect tab) then this will solve txns getting piled up. Also - I think the 2 min timeout works well - at least it worked for me.

@ryestew ryestew merged commit 5f08c8a into master Sep 11, 2020
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.

Integrate the ability of running provider plugins.

4 participants