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

Feat/message sign redo #708

Merged
merged 32 commits into from Apr 25, 2019

Conversation

@Schwartz10
Copy link
Contributor

commented Apr 16, 2019

No description provided.

@Schwartz10 Schwartz10 marked this pull request as ready for review Apr 16, 2019

@Schwartz10

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

Hey @sohkai I made this PR instead of trying to do any fancy git maneuvers in #683.

The conflicts were easy to fix, but this line is throwing an error when I start the app:

Wrapper init, fatal error: TypeError. Cannot read property 'subscribe' of undefined.

Was there anything introduced in aragon.js that is required to make this work? I pulled the latest master branch, so now I'm thinking i might need to just nuke all my node modules across all repos and start from scratch. Wanted to check with you first.

Everything else (tx's and signatures) are working well.

@Schwartz10 Schwartz10 referenced this pull request Apr 16, 2019
@sohkai

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

@Schwartz10 You'll want to remove package-lock.json and node_modules; the .subscribe() issue is because you haven't installed the correct @aragon/wrapper version.

@Schwartz10

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

Hey, I skimmed the code and tested things out again. Everything is working well! One UI thing I noticed (I think this was new after I worked all these merge conflicts out?):

image

The signing panel isn't big enough to fit the entire identity popup.

Is this something we should fix in this PR?

@sohkai

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

Hmm, that is quite weird. The popover should automatically adjust. Does it only happen in the messaging signing part of the panel or for other actions where the radspec string has an address (e.g. minting a token)?

@Schwartz10

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

The problem does not occur in the token manager

image

But it does occur in one of our other apps:

image

We'll look into it, but let me know if any ideas come to mind.

@Schwartz10

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

Investigation:

There are a few places where the css is set for modals in aragon/aragon:
https://github.com/aragon/aragon/blob/master/src/components/ModalManager/ModalManager.js
https://github.com/aragon/aragon/blob/master/src/components/ModalManager/Modal.js
https://github.com/aragon/aragon/blob/master/src/components/LocalIdentityModal/LocalIdentityModal.js

It wasn't obvious to me where the adjustment should be made, especially because I can't seem to find where the modal css is set in aragon/aragon-apps. What I did find out is that the LocalIdentityBagde and IdentityManager are different in aragon/aragon-apps and aragon/aragon.

aragon/aragon:
LocalIdentityBage
IdentityManager

aragon/aragon-apps:
LocalIdentityBage
IdentityManager

The IdentityContext.Provider has slightly changed - it now takes an additional onShowLocalIdentityModal prop, which currently is being passed to LocalIdentityModalProvider (in aragon/aragon instead of the IdentityContext.Provider). Changing this logic effects other parts of the app, for example, the AppIFrame component throws errors (I didn't look too much into why that's happening yet). So it wasn't as easy as just porting over the logic.

I don't think that the use of LocalIdentityBadge in the signing-panel is causing this weird UI, I think the modal css just needs to be tweaked (somewhere) or synced with the logic // styling in aragon-apps. Do you want me to file a separate ticket for this?

@luisivan luisivan referenced this pull request Apr 19, 2019
1 of 1 task complete
@bpierre
Copy link
Member

left a comment

Hey @Schwartz10, thanks for this PR! 🤗 I left a few comments.

For the position of the identity badge, it’s an issue related to both SidePanel and the way we use the Popper.js library. I am working on a fix, I’ll let you know when the update will be ready. After that, upgrading @aragon/ui will probably be the only thing needed to solve the problem.

src/components/SignerPanel/SignMsgContent.js Outdated Show resolved Hide resolved
src/components/SignerPanel/SignerPanel.js Outdated Show resolved Hide resolved
src/assets/arrow.svg Outdated Show resolved Hide resolved
import SignerButton from './SignerButton'
import ToggleContent from '../ToggleContent'
import LocalIdentityBadge from '../IdentityBadge/LocalIdentityBadge'
import AppInstanceLabel from '../../apps/Permissions/AppInstanceLabel'

This comment has been minimized.

Copy link
@bpierre

bpierre Apr 22, 2019

Member

We should probably move this component outside of apps/Permissions now that it is used from elsewhere.

This comment has been minimized.

Copy link
@Schwartz10

Schwartz10 Apr 23, 2019

Author Contributor

Ok, i did this in its own commit because it touches a lot of files. Would be really nice to try and get this in before others that touch the same components, so we don't have to keep fixing merge conflicts in this PR

This comment has been minimized.

Copy link
@bpierre

bpierre Apr 23, 2019

Member

Agreed, thanks!

src/components/SignerPanel/SignMsgContent.js Outdated Show resolved Hide resolved
<React.Fragment>
<SmMarginRight>
{'You are about to sign this message with the connected account '}
<LocalIdentityBadge entity={account} fontSize="xsmall" />

This comment has been minimized.

Copy link
@bpierre

bpierre Apr 22, 2019

Member

I just noticed that it was done in the actions paths and it’s probably why it’s done here as well, but I think we should avoid setting fontSize on this component until we have a version of it designed for inline content (coming soon!).

This comment has been minimized.

Copy link
@Schwartz10

Schwartz10 Apr 23, 2019

Author Contributor

Ok. I took this out, but FYI it looks a bit funky.
image

walletProviderId,
} = this.props

if (!hasWeb3) {

This comment has been minimized.

Copy link
@bpierre

bpierre Apr 22, 2019

Member

These checks (!hasWeb3 and !hasAccount) should probably be moved in the parent component, now that they are used in two places.

This comment has been minimized.

Copy link
@Schwartz10

Schwartz10 Apr 23, 2019

Author Contributor

You'll notice I added a component to make this check. The parent component was already very hefty (a conversation we had in the previous version of this PR), and doing so would (seemingly) require some nested ternaries.

a567218#diff-a6ba6f47578d687aea9e7b84f128d2c1R319
a567218#diff-fdbde9debbe8d9b21f3797ef3c251e6e

Alternatively, this added component could also be responsible for rendering either ConfirmMsgSign or ConfirmTransaction, but I like this style better because it should re-render less (less props will change) and seems like a nice separation of concerns.

This comment has been minimized.

Copy link
@bpierre

bpierre Apr 23, 2019

Member

Nice ❤️

Alternatively, this added component could also be responsible for rendering either ConfirmMsgSign or ConfirmTransaction, but I like this style better because it should re-render less (less props will change) and seems like a nice separation of concerns.

Agreed, I like it too :-)

src/utils.js Outdated Show resolved Hide resolved

bpierre and others added some commits Apr 22, 2019

Update src/utils.js
Co-Authored-By: Schwartz10 <Schwartz10@users.noreply.github.com>
Update src/components/SignerPanel/SignMsgContent.js
Co-Authored-By: Schwartz10 <Schwartz10@users.noreply.github.com>
@Schwartz10

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

I've resolved all the change requests, but still prob need tomorrow to test this one last time. I finished these changes on an airplane with bad internet

@bpierre

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

@Schwartz10 Awesome! For the popover, upgrading @aragon/ui to 0.36.0 should fix the issue.

@Schwartz10

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

Word! I think this is all set. :)

sohkai added some commits Apr 24, 2019

@sohkai

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

@Schwartz10 I've added a few commits, mostly to improve the prop-types and organization a bit:

  • 0de15f6: Move or rename a few components
  • 1c17177: It was in the design, but we've been trying to erase all colons from labels 😄
  • 400a766: Mostly prop-types changes and some code simplifications / cosmetic changes
  • ae35159: Improve the message signing panel's layout to have the vertical overflow and better handle the error that comes back when the user cancels a signature (oddly, Metamask gives us the entire stack trace in the message 🤷‍♂️)
@bpierre
Copy link
Member

left a comment

Looking good! One last thing that I just noted is that we don’t get anything from the app when the panel gets closed, which might be a problem if the app is in a waiting state.

Should we call signatureBag.reject(new Error("The panel was closed before signing the message")) in that case?

@Schwartz10

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

Hi @bpierre - I think that makes sense. Where did you envision that signatureBag.reject call getting made? Here? Is this happening for transaction handling as well?

@sohkai

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

Is this happening for transaction handling as well?

Yes... this was my first thought too. I've added the cancellation feedback for both in ad55b2d.

@bpierre
Copy link
Member

left a comment

💯

@sohkai sohkai merged commit ba10d27 into aragon:master Apr 25, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@aragon aragon deleted a comment from allcontributors bot May 4, 2019

@sohkai

This comment has been minimized.

Copy link
Member

commented May 4, 2019

@all-contributors please add @Schwartz10 for code

@allcontributors

This comment has been minimized.

Copy link
Contributor

commented May 4, 2019

@sohkai

I've put up a pull request to add @Schwartz10! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.