get certifications from BadgeReg, show them in accounts overview #3768

derhuerst · 2016-12-09T12:26:32Z

The PR makes the certifications/badges show up in the accounts overview and in the address book.

Triggered by the action fetchCertifiers, it uses BadgeReg.sol to fetch the addresses & metadata of all certifiers (contracts issuing certifications/badges). It then uses a filter to check for Confirmed events.

Still to do:

~~Listen for Revoked and remove the certification.~~
Consider renaming "certifications" to "badges". I kept it compatible with the rest of the code for now.

coveralls · 2016-12-12T11:57:53Z

Changes Unknown when pulling d67a938 on jr-use-badge-reg into ** on master**.

derhuerst · 2016-12-14T11:37:47Z

The tests run locally for me. CI seems to have issues.

coveralls · 2016-12-14T12:05:16Z

Changes Unknown when pulling 240bab2 on jr-use-badge-reg into ** on master**.

gavofyork · 2016-12-15T12:20:10Z

js/src/contracts/badgereg.js

-  fetchCertifier (name) {
-    if (this.certifiers[name]) {
-      return Promise.resolve(this.certifiers[name]);
+  nrOfCertifiers () {


nr -> number

(or just certifierCount)

jacogr · 2016-12-15T12:28:08Z

js/src/contracts/badgereg.js

+        return badgeReg.instance.badge.call({}, [ id ]);
+      })
+      .then(([ address, name ]) => {
+        if (address === ZERO20) throw new Error(`Certifier ${id} does not exist.`);


Considder putting this across 2 lines with a blank space between the blocks -

Readability, not gaining anything (I know beauty is in the eye of the beerholder)

When we add the tests and have coverage information, the line will show up as covered even when just the condition is evaluated - when it is in a proper block, the info will show that the error is actually tested for or now. (Goes for everything, even ?:).

jacogr · 2016-12-15T12:30:00Z

js/src/contracts/badgereg.js

+      .then(([ address, name ]) => {
+        if (address === ZERO20) throw new Error(`Certifier ${id} does not exist.`);
+        name = bytesToHex(name);
+        name = name === ZERO32 ? null : hex2Ascii(name);


Same split comment as above, would make life easier going forward, hence -

// not crazy about doing this, we do it in a couple of places // (I have started testing again BigNum(val).eq(0)) name = name === ZERO32 ? null : hex2Ascii(name);

jacogr · 2016-12-15T12:31:11Z

js/src/redux/providers/certifications/middleware.js

+        .then((logs) => {
+          logs.forEach((log) => {
+            const certifier = certifiers.find((c) => c.address === log.address);
+            if (!certifier) throw new Error(`Could not find certifier at ${log.address}.`);


Same single-line comment as above for the same reasons.

jacogr · 2016-12-15T12:32:00Z

js/src/redux/providers/certifications/middleware.js

+            const certifier = certifiers.find((c) => c.address === log.address);
+            if (!certifier) throw new Error(`Could not find certifier at ${log.address}.`);
+            const { id, name, title, icon } = certifier;
+            if (log.event === 'Revoked') {


Really picky, but would prefer a line before this block, just indicates a break between assigning and doing. (Not a crisis, just aids readability once again)

jacogr · 2016-12-15T12:33:02Z

js/src/redux/providers/certifications/middleware.js

-                if (isCertified) {
-                  const { name, title, icon } = cert;
-                  store.dispatch(addCertification(address, name, title, icon));
+      if (action.type === 'fetchCertifiers') {


Consider using a switch statement for action.type

jacogr · 2016-12-15T12:33:52Z

js/src/redux/providers/certifications/reducer.js

+    if (certifications.some((c) => c.id === id)) {
+      return state;
+    }
+    const newCertifications = certifications.concat({ id, name, icon, title });


linebreak before would be good, the rest of the callback is actually nice and readable.

jacogr · 2016-12-15T12:34:37Z

js/src/redux/providers/certifications/reducer.js

@@ -17,17 +17,25 @@
 const initialState = {};

 export default (state = initialState, action) => {
-  if (action.type !== 'addCertification') {
-    return state;
+  if (action.type === 'addCertification') {


switch statement would be good here as well, since we are only checking the type, default is the state return.

Changing it to a switch statement in this case means that the variable declarations aren't as local anymore. The linter will complain. Moving them up is ugly because we're assigning stuff we may not need.

jacogr · 2016-12-15T12:35:21Z

js/src/ui/Certifications/certifications.js

@@ -73,15 +64,12 @@ function mapStateToProps (_, initProps) {

  return (state) => {
    const certifications = state.certifications[account] || [];
-    return { certifications };
+    const dappsUrl = state.api.dappsUrl;
+    return { certifications, dappsUrl };


Less squashing, break would be good before the return line.

jacogr · 2016-12-15T12:35:43Z

js/src/ui/Certifications/certifications.js

 export default connect(
  mapStateToProps,
-  mapDispatchToProps
+  null


Could remove completely

I actually like null here because null !== undefined and because connect is commonly known as the function that accepts two params, one function for each direction.

jacogr · 2016-12-15T12:37:18Z

js/src/views/Accounts/List/list.js

@@ -31,7 +34,11 @@ export default class List extends Component {
    empty: PropTypes.bool,
    order: PropTypes.string,
    orderFallback: PropTypes.string,
-    handleAddSearchToken: PropTypes.func
+    certifications: PropTypes.object.isRequired,


Considder adding properties in alphabetical order, they are getting out of hand in some places. (I believe both myself and @ngotchac have been trying to do that lately just to bring in some sanity)

I don't have a system of ordering "regular" props, but I have at least been trying to put actions at the end and props passed from the parent at the beginning.

jacogr · 2016-12-15T12:39:16Z

Tiny grumbles & styling comments, none critical.

coveralls · 2016-12-15T13:49:51Z

Coverage increased (+0.3%) to 85.859% when pulling 4c42ded on jr-use-badge-reg into 03db4d0 on master.

derhuerst added 8 commits December 9, 2016 00:34

pull out fetchMeta

2b34d76

fetch certifiers by id

b32b636

fetch certifiers from BadgeReg

409c4ad

fetch certifications in account view

e1c5796

Certifications: read dappsUrl from state

5862f2a

show certifications in accounts list

a84cd91

fix linting issues

e536290

merge master into jr-use-badge-reg

3dc943e

derhuerst added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M7-ui labels Dec 9, 2016

derhuerst added 2 commits December 9, 2016 13:35

store id with certifiers

fd88421

remove certification on Revoke

6d20592

derhuerst changed the title ~~use BadgeReg to get list of certifications, display them in accounts overview~~ get certifications from BadgeReg, show them in accounts overview Dec 9, 2016

derhuerst added 2 commits December 9, 2016 16:53

merge master into jr-use-badge-reg

bc33231

merge master into jr-use-badge-reg

d67a938

merge master into jr-use-badge-reg

240bab2

derhuerst added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Dec 14, 2016

gavofyork reviewed Dec 15, 2016

View reviewed changes

merge master into jr-use-badge-reg

afba259

jacogr reviewed Dec 15, 2016

View reviewed changes

jacogr added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 15, 2016

derhuerst added 2 commits December 15, 2016 13:48

address style grumbles 💄

f59f7c5

sort props 💄

4c42ded

gavofyork merged commit d76239e into master Dec 15, 2016

gavofyork deleted the jr-use-badge-reg branch December 15, 2016 13:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

get certifications from BadgeReg, show them in accounts overview #3768

get certifications from BadgeReg, show them in accounts overview #3768

derhuerst commented Dec 9, 2016 •

edited

Loading

coveralls commented Dec 12, 2016

derhuerst commented Dec 14, 2016

coveralls commented Dec 14, 2016

gavofyork Dec 15, 2016

jacogr Dec 15, 2016

jacogr Dec 15, 2016 •

edited

Loading

jacogr Dec 15, 2016

jacogr Dec 15, 2016

jacogr Dec 15, 2016

jacogr Dec 15, 2016

jacogr Dec 15, 2016

derhuerst Dec 15, 2016

jacogr Dec 15, 2016

jacogr Dec 15, 2016

derhuerst Dec 15, 2016

jacogr Dec 15, 2016

derhuerst Dec 15, 2016

jacogr commented Dec 15, 2016 •

edited

Loading

coveralls commented Dec 15, 2016

get certifications from BadgeReg, show them in accounts overview #3768

get certifications from BadgeReg, show them in accounts overview #3768

Conversation

derhuerst commented Dec 9, 2016 • edited Loading

coveralls commented Dec 12, 2016

derhuerst commented Dec 14, 2016

coveralls commented Dec 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacogr Dec 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacogr commented Dec 15, 2016 • edited Loading

coveralls commented Dec 15, 2016

derhuerst commented Dec 9, 2016 •

edited

Loading

jacogr Dec 15, 2016 •

edited

Loading

jacogr commented Dec 15, 2016 •

edited

Loading