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

Add optional showPopup parameter to .enable() #6045

Closed
wants to merge 5 commits into from

Conversation

wbt
Copy link
Contributor

@wbt wbt commented Jan 17, 2019

This is a rough start at addressing Issue #6022. I believe it still needs work and testing, but can't do more at this moment so I'll share the start and explanation in the hopes someone might be able to pick up and progress further.

Tracing:

I see the provider getting an enable function with a {force} parameter here.

That function sends a ETHEREUM_ENABLE_PROVIDER message to the window along with the force parameter.

That message is caught and forwarded as init-provider-request which is caught and passed to _handleProviderRequest with the force parameter.

A key line seems to be

if (!force && this.approvedOrigins[origin] && this.caching && isUnlocked) {

which if true, returns approval and otherwise opens a popup. So it seems the "force" parameter is used to force a popup even when it might not otherwise be needed. The parameter requested here is in some ways the opposite of that.

In _handleProviderRequest the only onward options are to grant quick approval or open a popup, with no quick denial option. One way to implement this request might be to add such an onward option in an else block following the openPopup line after wrapping that line in an if(popup) block.

As a side note, openPopup is passed in here from here, apparently defined here, which just opens a previously defined notification popup.

That onward option might make use of _handleIsApproved which sends an answer-is-approved message along with a boolean isApproved parameter. That message gets caught and forwarded as ethereumisapproved with the boolean, This message is caught and passed to isApprovedHandle. A better strategy is probably to use rejectProviderRequest in a similar manner to the use of approveProviderRequest.

This pull request uses that latter strategy; see new lines 65-70 of this commit. The other commits are just trying to get the parameter value passed through to the place where it can be used.

@wbt wbt changed the title Patch 2 Add optional showPopup parameter to .enable() Jan 17, 2019
@danfinlay
Copy link
Contributor

Hi, thanks for taking the time to draft this. Sorry we didn't reply to & discuss your proposal before you put the coding time in.

We are currently working on a big update to how dapps are signed in to, and it is going to affect when prompts are shown, and will include a better way of checking current permissions.

We could definitely add this now, and then polyfill its behavior under the new refactor, but it means we'll have a bit more polyfill code we end up injecting into every page for backwards compatibility, so I'm a little reluctant for that reason.

We should have something we can tangibly share and discuss in about two weeks, if you can wait that long on this, I think it will satisfy the needs that inspired this issue & PR.

@wbt
Copy link
Contributor Author

wbt commented Jul 18, 2019

@danfinlay Thanks for responding.

Sorry we didn't reply to & discuss your proposal before you put the coding time in. We are currently working on a big update...

This PR was silently blocked by secret plans made in closed-door sessions that aren't even pointed out until 6 months after the proposal. It is another illustration of how "open-source" projects core to development in the present-day blockchain ecosystem are not truly open; see discussion on this issue for more detail.

The response on this definitely discourages community collaboration and contribution to the project.

@danfinlay
Copy link
Contributor

I'm really sorry for making it feel that way. Blockchain is so full of potential for open collaboration, it would be great if all the constituent parts were as open as our shared ledger. I feel this deeply, and my goals for the product are to increase (via API) the amount that developers like you can do without our central coordination (because it's so limited!).

While we can provide lots of automated collaboration via API, when it comes to our core architecture and plans, we're just human, we're often bottle-necked, and just doing our best to focus on the most important things we can. Especially sorry for missing your issue 6 months ago.

Anyways, I didn't even mean to say this was definitely blocked. I'm just saying we have a more holistic approach to checking permissions than adding a new boolean to the enable options, and if you can give us a couple more weeks, you might agree.

@whymarrh
Copy link
Contributor

Again, apologies for the delay in getting back to you on this—we're going to be better at responding to incoming PRs going forward.

@whymarrh whymarrh closed this Jul 19, 2019
@wbt
Copy link
Contributor Author

wbt commented Aug 19, 2019

Anyways, I didn't even mean to say this was definitely blocked.

It's been "closed with unmerged commits" with no explicit indication that anything else specific is taking its place to address the same underlying issue. (I'm not saying these commits specifically should be merged without further review/work as needed, though this PR underscrores the last sentence of point 1 here.)

We should have something we can tangibly share and discuss in about two weeks, if you can wait that long on this, I think it will satisfy the needs that inspired this issue & PR. ...
I'm just saying we have a more holistic approach to checking permissions than adding a new boolean to the enable options, and if you can give us a couple more weeks, you might agree.

We’ve waited the requisite two weeks, twice, and more beyond; this seems to stretch the qualifier “about” past breaking. The core cause of the frustration expressed above is the response, common to hear from core blockchain technology projects today, that “the project cannot advance in some particular way because of a decision we made as core team members in a closed meeting, and we’re not going to explain the reasoning for why we arrived at that decision in an open on-point setting like the relevant GitHub issue.” On a longer thread about the frustration induced by these responses, I wrote:

When I first learned about how open-source works, I was taught that any discussions in a more rapid synchronous channel (like an in-person meeting or teleconference) should be summarized on the archival listserv (now, it would be GitHub Issues), including who was there, what decisions were made, the reasons for them, and reasons for an alternative path that were considered but overridden. I would like to see more of that happening in these projects.

I hereby write the same thing, about MetaMask.

@bdresser
Copy link
Contributor

@wbt Sorry to hear all this. Obviously we're spending a lot of time going back-and-forth with you across various issues and PRs. Could you please DM me an email address or other contact info? I'd love to speak with you "in a more rapid synchronous channel" to help make sure we're establishing a healthy relationship where you're feeling heard and the project is benefitting from your efforts. bobby@metamask.io. Thanks!

@wbt
Copy link
Contributor Author

wbt commented Aug 21, 2019

@bdresser Thanks for the response. However, I think that responding as requested would undermine the main point I am trying to make:

Too many repositories of technologies that are core to blockchain development are each governed (e.g. decisions about their development are made) by a small group of core developers in closed-door meetings/channels without even having the reasons for those decisions publicly available.

This is poor practice in stewardship of an open-source community and exceptionally poor practice in stewardship of a community that is supposed to be about facilitating decentralization, and/or pseudonymous reliable interactions. Because I believe there is value in the vision, I invest time in trying to change this, including by pointing out examples where the issues are demonstrated, like this thread. MetaMask can do better.

I accept that I'm not on the core team of MetaMask, not able to commit directly, not part of the discussions where decisions about MetaMask development are made, and not receiving any funding from ConsenSys or dev grants supporting MetaMask. Even if I had sufficient available time and an invitation for any of that to change that for me specifically (which is not how I interpret the last comment; this is even if), that wouldn't change the core issue of transparency for the community as a whole.

to help make sure... the project is benefitting from your efforts.

I think the project is getting some benefit from my efforts, especially some of the detailed Issue descriptions that I invest a significant amount of time in. (If not, that'd be good to find out ASAP and I'll stop wasting time on it including a switch to option 3 here.)

I believe the project would benefit more from contributions by me and likely others if there were more transparency and a tad bit less NIH syndrome. PR handling like this one is very discouraging to potential contributions.

The substantive close reason was "We are currently working on a big update...We should have something we can tangibly share and discuss in about two weeks... I think it will satisfy the needs that inspired this issue & PR." After five, are we there yet? Is there something "we can tangibly share and discuss?" If so, please post a description or at least a link below.

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