-
Notifications
You must be signed in to change notification settings - Fork 68
Conversation
@pelle quick note: I'll get 1. tests working, and 2. update MetaIdentityManager to take into account the recent changes in IdentityManager. Should be pretty simple. I'll update you when ready for reveiw :). Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Let's just update the tests to use async/await everywhere for consistancy.
test/txRelay.js
Outdated
|
||
function signPayload(signingAddr, sendingAddr, txRelay, destinationAddress, functionName, | ||
functionTypes, functionParams, lw, keyFromPw) { | ||
return new Promise( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
await could be used in this function instead.
test/txRelay.js
Outdated
|
||
errorThrown = false | ||
|
||
MetaTxRelay.new().then(instance => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using await should work here as well
Also: we need to add |
@oed changed tests to await. Added files to index.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! async/await is so nice! :)
contracts/MetaIdentityManager.sol
Outdated
} | ||
|
||
modifier onlyOwner(address identity, address sender) { | ||
if (owners[identity][sender] > 0 && (owners[identity][sender] + userTimeLock) <= now ) _ ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to use isOwner
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think this makes sense for clarity. I will update now. Here and in IdentityManager
fa5bc58
to
e45ce2f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM great team work @oed, @naterush and @christianlundkvist
Ok, merging! 😃 |
This pull request simplifies the meta-tx pull request dramatically (and contains both the IdentityManager from the MetaIdentityManager). For the write-up, still see here: https://consensys.quip.com/CoClAjSTyBZI