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 signing #683

Closed
wants to merge 64 commits into from

Conversation

Projects
None yet
6 participants
@Schwartz10
Copy link

Schwartz10 commented Apr 5, 2019

No description provided.

drcmda and others added some commits Jan 17, 2019

Merge remote-tracking branch 'origin/master' into notifications
* origin/master: (55 commits)
  Identity - Improve LocalIdentityBadge (#673)
  Menu panel footer separator (#666)
  fix(MenuPanel): avoid clickable margin above system apps toggle (#671)
  Local identities (#656)
  MenuPanel: add toggle animation to show/hide system apps (#658)
  Add github workflow for linting/building (#663)
  Permissions: added system and background app labels (#650)
  Use the same component to render every app icon (#655)
  chore: add all contributors and contributing guidelines (#649)
  Manage the menu button using messages + prevent re-mounting on resize (#651)
  Update melon (#647)
  Apps <> System apps separator (#648)
  Upgrade lint-staged (#646)
  fix: always leave Kernel as first app (#645)
  fix: avoid assigning a registry tag if app is not on a registry (#644)
  chore: upgrade @aragon/wrapper to v4.0.0-beta.1 (#639)
  DaoSettings: add bottom margin on app items (#638)
  Refactor common components (#615)
  Enforce MenuPanel’s width (#636)
  Menu panel swipe open close (#606)
  ...
Move activities to context from wrapper state
- Rename notifications to activities
- Pass activity count down the tree
Fix bug with activity panel not closing properly
The callback for toggling activity panel was called twice
once by notification alert component and once by the onBlur event with
a short delay. This caused it to start closing and then to open again.
@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Apr 5, 2019

CLA assistant check
All committers have signed the CLA.

Show resolved Hide resolved src/Wrapper.js Outdated

@Schwartz10 Schwartz10 force-pushed the openworklabs:feat/message-signing branch from cfb7013 to dedafed Apr 8, 2019

Schwartz10 added some commits Apr 11, 2019

@@ -174,16 +242,18 @@ class SignerPanel extends React.Component {
status,
} = this.state

const isTxSignReq = isTxSignerStatus(status)

This comment has been minimized.

Copy link
@2color

2color Apr 12, 2019

Contributor

It's not so obvious what this is. Given that there are two modes, namely

  • message signature
  • transaction

What do you think about naming it simply isTransaction Signing request is already implied by the component name an.

Alternatively, breaking the status types from:
STATUS_CONFIRMING_TX, STATUS_SIGNING_TX, STATUS_SIGNED_TX, STATUS_ERROR_TX, STATUS_CONFIRMING_MSG_SIGN, STATUS_SIGNING_MESSAGE, STATUS_MESSAGE_SIGNED, STATUS_ERROR_SIGNING_MSG,

to two separate data types, e.g.mode and status might reduce the complexity given that both signatures and transactions follow the same state machine model.

This comment has been minimized.

Copy link
@Schwartz10

Schwartz10 Apr 13, 2019

Author

I like isTransaction! Latest commit has that change.

Would you also like me to implement the mode status logic instead of the 4 separate statuses?

@2color

2color approved these changes Apr 12, 2019

Copy link
Contributor

2color left a comment

Looking good both from visually and code-wise. I left some remarks as food for thought.

I'm still thinking if we're not stuffing too much into the SignerPanel as it is; perhaps it could use another level of abstraction between transaction signing and message signing.

I don't have any strong opinion on that. I think SignerPanel can be simplified as is without necessarily abstracting into separate components.

2color and others added some commits Apr 12, 2019

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

This comment has been minimized.

Copy link
Member

sohkai commented Apr 13, 2019

@Schwartz10 It would be great if you could merge this branch with #687 and work off of it. Just tried merging the two and there were quite a few conflicts.

@Schwartz10

This comment has been minimized.

Copy link
Author

Schwartz10 commented Apr 13, 2019

Hey @sohkai and @2color ! We're pretty close!

One question, how should we handle the situation when a developer calls app.requestSignMessage() without a message? Should that throw an error? If yes, should the error be formed in aragon/aragon or rejected right away in aragon/api?

It seems like calling signatureBag.reject with a descriptive error message might be a good place. But if aragon.js is the proper place, I'm happy to submit another little PR there.

@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Apr 14, 2019

how should we handle the situation when a developer calls app.requestSignMessage() without a message

You should be allowed to sign an empty message; we could just display in the UI that there's no message included.

@Schwartz10

This comment has been minimized.

Copy link
Author

Schwartz10 commented Apr 15, 2019

You should be allowed to sign an empty message; we could just display in the UI that there's no message included.

understood. But what actually is an empty message? An empty string? Attempting to sign undefined or null throws an error.

Also @sohkai i just took a look at these most recent merge conflicts. They're not too bad, but it seems likely some were introduced after merging #687. Do you want me to work those out? Or just sit tight until #674 gets merged?

@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Apr 15, 2019

But what actually is an empty message?

Ahh, that's a good point, I was just assuming ''. This sounds like something that would fit best under @aragon/wrapper; we don't do a lot of error checking in @aragon/api (e.g. you could send null through as an intent).

Or just sit tight until #674 gets merged?

Probably easiest to wait a bit for this to get merged so there's less churn. Is everything ready on your side for a review and merge?

@Schwartz10

This comment has been minimized.

Copy link
Author

Schwartz10 commented Apr 15, 2019

I was just assuming ''

Right, this is working as-is now

This sounds like something that would fit best under @aragon/wrapper

It doesn't sound horrible to reject the signatureBag if aragon/aragon receives null or undefined. Happy to add this if you think it's a good idea, or work error handling into @aragon/wrapper. Whatever you prefer.

I think this is all set! I'll have one more look over after #674 makes it into master, since it appears as tho i've made many changes that I haven't.

@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Apr 15, 2019

I think it'll make most sense to add it as an error in @aragon/wrapper (it can just throw in signMessage()). Does web3.eth.personal.sign() accept only strings or any JSON-serializable object?

@Schwartz10

This comment has been minimized.

Copy link
Author

Schwartz10 commented Apr 15, 2019

I can't seem to find documentation on it, but I just ran a quick test and signed two different objects, and they both came back with the same signatures.

Not entirely sure what's going on there, but i think it might be smart to try and enforce passing strings?

@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Apr 15, 2019

It might be smart to try and enforce passing strings

Sounds reasonable to me 😄.

@Schwartz10

This comment has been minimized.

Copy link
Author

Schwartz10 commented Apr 16, 2019

@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Apr 16, 2019

@Schwartz10 activity-panel is now merged 🎉! Let's get this merged with master and reviewed :).

@Schwartz10

This comment has been minimized.

Copy link
Author

Schwartz10 commented Apr 16, 2019

I wonder if I would have less merge conflicts if I plucked this commit out of this PR: f24975a. Still says I've changed 28 files, plus I don't believe I ever touched:

src/components/Activity/ActivityAlert.js
src/components/Activity/ActivityItem.js
src/components/Activity/ActivityList.js
src/components/Activity/ActivityPanel.js 

Thoughts?

@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Apr 16, 2019

@Schwartz10 could be, if it was an old merge... Those were probably from merging with the in-progress activity-panel PR :(.

@Schwartz10

This comment has been minimized.

Copy link
Author

Schwartz10 commented Apr 16, 2019

Closing this for #708

@Schwartz10 Schwartz10 closed this Apr 16, 2019

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