gitcoinco / web

Grow Open Source
https://gitcoin.co
Other
1.78k stars 773 forks source link

Update Gitcoin for breaking MetaMask changes #1964

Closed owocki closed 6 years ago

owocki commented 6 years ago

Please read this medium article before reading this desc.

https://medium.com/metamask/https-medium-com-metamask-breaking-change-injecting-web3-7722797916a8

OCTOBER edit: heres a new post from metamask about how to do this: https://medium.com/metamask/eip-1102-preparing-your-dapp-5027b2c9ed76

The scope of this ticket is to update Gitcoin for the above breaking metamask changes.

Please submit a PR with the updates in it; Make sure you've tested

before you submit the PR

gitcoinbot commented 6 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


Work has been started.

These users each claimed they can complete the work by 1 week, 6 days from now. Please review their action plans below:

  1. zevaverbach has started work.

    I will replace all direct reliance on injected web3 with a check to request permission and initialize web3 manually.

Learn more on the Gitcoin Issue Details page.

gitcoinbot commented 6 years ago

💰 A crowdfund contribution worth 0.10000 ETH (36.53 USD @ $365.27/ETH) has been attached to this funded issue from @dmvt.💰

Want to chip in also? Add your own contribution here.

zevaverbach commented 6 years ago

@owocki as expected, the web3 object is all over the place in the js files. Are the five events described in the issue the only events ever facilitated on Gitcoin?

owocki commented 6 years ago

Hey Zev, yes to documentation if you find it helpful to document what you learn!

Are the five events described in the issue the only events ever facilitated on Gitcoin?

I don't believe so, no. But they are the five most major use cases right now.

I wonder if instead of trying to migrate away all the global variables, we instead use some of the code that proimps the user to 'unlock' their metamask when there is no web3 enabled, to instead 'enable Gitcoin on their Metamask? This would probably be a much cleaner way to do this.

zevaverbach commented 6 years ago

@owocki sounds good, I'll take this approach

zevaverbach commented 6 years ago

@owocki "some of the code that prompts the user...": By this do you mean some of Metamask's code? I'm only seeing a static message "Please Unlock Metamask to Continue" in web, not a prompt.

zevaverbach commented 6 years ago

All right, this pending Metamask PR seems like it will provide the needed UI component. https://github.com/MetaMask/metamask-extension/pull/4703

zevaverbach commented 6 years ago

opened WIP PR #1992, making progress. will move whining to there. 😆

owocki commented 6 years ago

@owocki one more comment here: Without making sure web3.js is included in several pages in addition to tips + bounties -- such as faucet -- web will break with respect to Metamask because the Web3 class won't be available. Outside the scope of this issue, but FYI.

yea i agree this is an issue. seems like it should be fairly trivial to just include web3.js 0.20.3 as a dependancy on gitcoin and set metamask to the provider if it exists. i believe this PR ( https://github.com/gitcoinco/web/pull/985 ) does this already!

gitcoinbot commented 6 years ago

@zevaverbach Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

gitcoinbot commented 6 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


@zevaverbach due to inactivity, we have escalated this issue to Gitcoin's moderation team. Let us know if you believe this has been done in error!

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

gitcoinbot commented 6 years ago

@zevaverbach Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

gitcoinbot commented 6 years ago

@zevaverbach Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

gitcoinbot commented 6 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


@zevaverbach due to inactivity, we have escalated this issue to Gitcoin's moderation team. Let us know if you believe this has been done in error!

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

PumpkingWok commented 6 years ago

Hi @owocki, I would like to ask you a question regard this issue. From the article that you linked (https://medium.com/metamask/https-medium-com-metamask-breaking-change-injecting-web3-7722797916a8), edited on 9/13, there is follow code:

window.addEventListener('load', async () => {
    // Modern dapp browsers...
    if (window.ethereum) {
        window.web3 = new Web3(ethereum);
        try {
            // Request account access if needed
            await ethereum.enable();
            // Acccounts now exposed
            web3.eth.sendTransaction({/* ... */});
        } catch (error) {
            // User denied account access...
        }
    }
    // Legacy dapp browsers...
    else if (window.web3) {
        window.web3 = new Web3(web3.currentProvider);
        // Acccounts always exposed
        web3.eth.sendTransaction({/* ... */});
    }
    // Non-dapp browsers...
    else {
        console.log('Non-Ethereum browser detected. You should consider trying MetaMask!');
    }
});

If i understood well in this way the code should be ready also for changing that will be introduce in November, but before that date it is impossible to test the first if condition (if (window.ethereum)), because the ethereum object does not exist yet. right ?

How would you like the code to be implemented ? I don't understand well if the code initialize only one instance of web3. I have seen also that you use Web3 python package as python side, right ?

Thanks in advance.

owocki commented 6 years ago

@PumpkingWok great question... i just asked them on medium and on the consensys internaly slack. pasting my message below

hey metamask team, 

the gitcoin team is hard at work preparing for this breaking change scheduled to rollout on november 2nd.: https://medium.com/metamask/https-medium-com-metamask-breaking-change-injecting-web3-7722797916a8

question for you : do you plan to release a version of metamask that we can test our changes with, prior to november 2nd?  does this already exist on your github?  if so, could you point us to the branch/tag?

thanks
:gitcoin: team
owocki commented 6 years ago

@PumpkingWok from metamask team:

next release: https://github.com/MetaMask/metamask-extension/pull/5256
10% rollout at the moment, should be receiving it soon :tm:
PumpkingWok commented 6 years ago

hi @owocki, Thank you very much for the info.

How would you like to proceed ? while i'm waiting the update i can look again into your code. I did not understand well all places in the code where i need to edit it.

I have seen listen_to_web3_changes() that calls currentNetwork() inside v2/js/shared.js. Is it the right place for check the network changes ? Thanks

owocki commented 6 years ago

How would you like to proceed ?

you can download the new metamask and use that for testing.

Is it the right place for check the network changes ?

i dont know enough about the new architecture of metamask to answer this question.

PumpkingWok commented 6 years ago

Hi, Thank you for the reply,i’m going to download it. For another question i mean in your code not in metamask.i’m not sure to understood well in your code all places where it checks for metamask connection.thanks

PumpkingWok commented 6 years ago

Hi @owocki, I was testing a new ethereum command from web console but it seems disappear from today. I have just seen that metamask released a new version (4.11.0) and it reverts to version 4.9.3. I think that the new command is disappear for this reason. Thanks

PumpkingWok commented 6 years ago

Hi @owocki, Metamask just released a new version, adding back ethereum object. I'm back to this issue, thanks.

owocki commented 6 years ago

great!

gitcoinbot commented 6 years ago

@PumpkingWok Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

gitcoinbot commented 6 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


@PumpkingWok due to inactivity, we have escalated this issue to Gitcoin's moderation team. Let us know if you believe this has been done in error!

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

owocki commented 6 years ago

let me snooze gitcoinbot for a bit.. since we've agreed to come back to this in 10 days time

PumpkingWok commented 6 years ago

Hi @owocki, Yes sure, no problem. I did not work in it for some days because Metamask had released a new version again without ethereum object. From yesterday the last version released bring back ethereum.I'm going to work in it deeply from tomorrow. Note: The UI for user Accept/Reject has not been released yet in official version.

zevaverbach commented 6 years ago

@owocki, it seems like @pumpkingwok is still working on this. Should I hold off? Also happy to pair on it, @pumpkingwok.

Zev Averbach Averbach Transcription (802) 276-5934 avtranscription.com http://www.avtranscription.com/

On Mon, Oct 15, 2018 at 12:53 PM, Kevin Owocki notifications@github.com wrote:

let me snooze gitcoinbot for a bit.. since we've agreed to come back to this in 10 days time

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/gitcoinco/web/issues/1964#issuecomment-429930848, or mute the thread https://github.com/notifications/unsubscribe-auth/ACtRNOtifTD2bKYhJ2zh5P61r0APg3GJks5ulL2kgaJpZM4V0QKx .

owocki commented 6 years ago

@zevaverbach has a PR out @PumpkingWok FYI - https://github.com/gitcoinco/web/pull/1992 -- recommend that if you start on this, you collaborate on his branch. i'm happy to pay ya both.

@zevaverbach 's current plan is to revisit his PR in a week or two, did i get that correct?

PumpkingWok commented 6 years ago

Hi @owocki, I did not know it, i asked it before start the work.I'm going to leave the work, ok ?

zevaverbach commented 6 years ago

Sounds good, I'll press on with it on the agreed upon timeline.

Zev Averbach Averbach Transcription (802) 276-5934 avtranscription.com http://www.avtranscription.com/

On Mon, Oct 15, 2018 at 2:45 PM, andrea zuccarini notifications@github.com wrote:

Hi @owocki https://github.com/owocki, I did not know it, i asked it before start the work.I'm going to leave the work, ok ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/gitcoinco/web/issues/1964#issuecomment-429969661, or mute the thread https://github.com/notifications/unsubscribe-auth/ACtRNDwYsLCGydGpJu75592RXpd_M3trks5ulNfQgaJpZM4V0QKx .

PumpkingWok commented 6 years ago

I stopped the work on this issue

owocki commented 6 years ago

heres a new post from metamask about how to do this: https://medium.com/metamask/eip-1102-preparing-your-dapp-5027b2c9ed76

pinkiebell commented 6 years ago

I'm ready to take this on - if needed 👍

gitcoinbot commented 6 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


Work has been started.

These users each claimed they can complete the work by 6 months, 2 weeks ago. Please review their action plans below:

1) usmanmuhd has started work.

I will go through the blog and hope to turn this in at the earliest.

Learn more on the Gitcoin Issue Details page.

usmanmuhd commented 6 years ago

@owocki I was thinking of having a Allow Gitcoin to access Metamask kind of button on the top, in the nav bar. Basically you need to authenticate first. If unauthenticated, the flow should be handled just the way it is done for Metamask locked albeit with a different message. The problem with this approach is that will it persist to the tips page and so on. PS: I also installed Metamask using the guide in the blog. It does not give me a pop up to authenticate when I run window.ethereum.enable() but it enables straight away. Can this be checked please?

PumpkingWok commented 6 years ago

Hi @usmanmuhd, Unfortunately if you want test the new UI and the privacy mode you have to install the latest custom build (https://github.com/MetaMask/metamask-extension/pull/4703#issuecomment-430814765), i think that metamask will implement the new features in the next release.

If i understood well there will be three phases: 1) From November 2 Metamask will set the privacy mode OFF by default, it means that the current gitcoin web code (using web3) will work without problems. In any case you should already prepare the code for using ethereum.enable(), if the privacy mode is OFF executing this command will expose the user info in automatic way, like now with window.web3. However a user could turn ON the privacy mode and in this case you have to already be ready with new code.

2) After some times (maybe few weeks/months) Metamask will set the privacy mode ON by default, at that time you have to use only ethereum.enable() if you don't want to turn OFF again the privacy mode. In this case when you call ethereum.enable() the UI will appear for choosing if approve or reject the account expose. The UI will appear only once if you approve, you can reset the approved websites using CLEAR PRIVACY DATA in settings.

3) In the last phase Metamask will delete the privacy mode completely, from this phase you need to use only ethereum.enable() and the user have to Approve/Reject for any Dapp.

I have just implemented the same issue for Zeppelin ethernaut https://github.com/OpenZeppelin/ethernaut/pull/87

Ask me without problems if you will encounter issues, have a nice day !!!

usmanmuhd commented 6 years ago

@PumpkingWok I installed the custom build itself. I even enabled the privacy mode. But still I do not get a UI asking for approval. It does work as expected (i.e. does not work without calling ethereum.enable()). I'll message you on Slack if I have further questions. Thanks!

PumpkingWok commented 6 years ago

Hi @usmanmuhd, What build have you tried ? cf02009 or 634121d ? I have had no issue, how did you try it ? You can use console to test the UI in easy way.

Thanks.

usmanmuhd commented 6 years ago

Hey @PumpkingWok I tried cf02009. I opened a local gitcoin instance (localhost:8000) and it showed Metmask unlocked even after I unlocked. So I ran window.ethereum.enable(), and then it started working fine. What I am saying is I did not get a pop up asking for authentication. Is it intended that way or an error on my side? Edit: Uninstalling and reinstalling Metamask somehow fixed the error :man_facepalming: Thanks!

PumpkingWok commented 6 years ago

Hi, Perfect, i'm happy that you resolved it. In any case ask me without problems !!

zevaverbach commented 6 years ago

@owocki confirming that @usmanmuhd is now working on this, so i wont proceed with doing so this weekend.

On Wed, Oct 24, 2018 at 10:40 PM Gitcoin.co Bot notifications@github.com wrote:

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work has been started.

These users each claimed they can complete the work by 2 months ago. Please review their action plans below:

1) usmanmuhd https://gitcoin.co/profile/usmanmuhd has started work.

I will go through the blog and hope to turn this in at the earliest.

Learn more on the Gitcoin Issue Details page https://gitcoin.co/issue/gitcoinco/web/1964/924.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/gitcoinco/web/issues/1964#issuecomment-432894014, or mute the thread https://github.com/notifications/unsubscribe-auth/ACtRNJ3ISUHe2RTbo_3qFN0w3bVJpZD4ks5uoSSbgaJpZM4V0QKx .

-- Zev Averbach Averbach Transcription (802) 276-5934 avtranscription.com http://www.avtranscription.com/

owocki commented 6 years ago

@zevaverbach thanks for the update.. @usmanmuhd can you throw up an ngrok or stage server where we can test your code?

usmanmuhd commented 6 years ago

@owocki will set it up today.

usmanmuhd commented 6 years ago

@owocki Setup a stage server on http://142.93.210.97:8000/

owocki commented 6 years ago

@usmanmuhd thanks.. checking it out now

owocki commented 6 years ago

hmmm im having a hard time testing because my chromium isnt playing nice with the new test repo..

@thelostone-mc or @SaptakS are you able to test?

thelostone-mc commented 6 years ago

@owocki this is from the PR

The issues :

untitled

usmanmuhd commented 6 years ago

@thelostone-mc I am not sure on how to add the logo. Will take a look at it. Regarding the first issue: I was thinking of making the user have an explicit button click to show the approval dialog. There seems to be a window.ethereum._metamask.isApproved() method, but this always returns false in my case. Could you please check this on your system? If this is implemented properly, then the flow will be: First time user: button click -> approve ->works normally Returning user: silent approval -> works normally

thelostone-mc commented 6 years ago

@usmanmuhd it looks like that works ! For now -> we could add the button to settings/account under Account Preferences (as I'm can't seemt to find a better place for it )

Could you write up a button which does that for now ? As simple as If window.ethereum._metamask.isApproved() is true -> metamask is enabled else text "Allow Gitcoin to use Metamask" and a button.

Would you be able to wrap that up ?

screen shot 2018-11-05 at 6 43 06 pm

cc @willsputra If you have a design in mind for this -> I'll fix up the css once @usmanmuhd gets the functionality out ^_^