-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Conversation
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.
Few grumbles.
js/src/ui/DappVouchFor/store.js
Outdated
this._api = api; | ||
this._app = app; | ||
|
||
Contracts |
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.
That won't really work in case of re-connection or dynamic change, but I supposed we don't need that for initial PoC.
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.
100%, added to list of next PR cleanups.
js/src/ui/DappVouchFor/store.js
Outdated
Contracts | ||
.get().registry | ||
.lookupAddress('vouchfor') | ||
.then((address) => { |
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.
Why not write everything as async
function and simplify constructor to only do:
constructor() {
this.initialize().catch(...);
}
async initialize() {
...
}
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.
Tried it, eslint didn't like that approach at all. (But probably due to the fact that I mixed promises/async as I still do here instead of fully async function).
Good point, added to the list of cleanups.
} | ||
|
||
@action addVoucher = (voucher) => { | ||
this.vouchers = uniq([].concat(this.vouchers.peek(), [voucher])); |
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.
this.vouchers.peek()
?
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.
MobX idisyncracy - arrays are actually objects, .peek() returns the actual observed array. If no peek, basically it concats an object.
app: PropTypes.object.isRequired | ||
}; | ||
|
||
store = new Store(this.context.api, this.props.app); |
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.
TBH I'm pretty sure it will lead to a broken behaviour.
The component instance can be re-used by react and just receive new props, should re-initialize Store
on componentWillReceiveProps
if we want to do it right.
(side note: it will also do some excessive calls for each dapp, might be worth to only limit it to network dapps?)
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.
Good point. It is not optimal usage-wise atm, that will make it better for at least the intial version.
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.
Edit: added only for apps where we have an actual contentHash
* Remove .only * Add vouch overlays to dapps * Cleanup address * Only run where we have a contentHash * GitLab kickstart
Add overlay of accounts vouching for a specific dapp, along with count of vouchers.
TODO: Can be optimised - a lot.