Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Display vouched overlay on dapps #6710

Merged
merged 5 commits into from
Oct 11, 2017
Merged

Display vouched overlay on dapps #6710

merged 5 commits into from
Oct 11, 2017

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Oct 11, 2017

Add overlay of accounts vouching for a specific dapp, along with count of vouchers.

parity 2017-10-11 17-16-45

TODO: Can be optimised - a lot.

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. B0-patch labels Oct 11, 2017
@jacogr jacogr requested a review from gavofyork October 11, 2017 15:27
Copy link
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few grumbles.

this._api = api;
this._app = app;

Contracts
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Contracts
.get().registry
.lookupAddress('vouchfor')
.then((address) => {
Copy link
Collaborator

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() {
 ...
}

Copy link
Contributor Author

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]));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.vouchers.peek()?

Copy link
Contributor Author

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);
Copy link
Collaborator

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?)

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

@5chdn 5chdn added this to the Patch milestone Oct 11, 2017
@5chdn 5chdn mentioned this pull request Oct 11, 2017
67 tasks
@tomusdrw tomusdrw added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 11, 2017
@jacogr jacogr merged commit 7feb82c into master Oct 11, 2017
@jacogr jacogr deleted the jg-dapp-vouched branch October 11, 2017 19:05
jacogr added a commit that referenced this pull request Oct 11, 2017
* Remove .only

* Add vouch overlays to dapps

* Cleanup address

* Only run where we have a contentHash

* GitLab kickstart
arkpar pushed a commit that referenced this pull request Oct 11, 2017
* [ci skip] js-precompiled 20171011-183908

* Fix estimate gas if from is not provided. (#6714)

* Display vouched overlay on dapps (#6710)

* Remove .only

* Add vouch overlays to dapps

* Cleanup address

* Only run where we have a contentHash

* GitLab kickstart
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants