implement basic badges/certifications/flair #3665

derhuerst · 2016-11-29T10:54:12Z

This PR is a first take on #2156.

In the Account page, the badges are shown below the tokens, in a similar style.

The redux middleware fetches the addresses of all individual badge contracts from BadgeReg.sol and expects all of them to implement the Certifier.sol interface. SMSVerification.sol is one of them. The badge icons get fetched from GitHubHint.sol, based on the IMG meta field in BadgeReg.sol.

In the code, this functionality is called "certifier" right now. I'm not really happy with that, will consider changing it. The term "badge" is blocked however by Material UI.

The current styling wastes a lot of space. I'm also not sure wether the badges should or should not be displayed in the style of tokens.

To play with it on ropsten:

0x01e1a37118Fe3BeFd17C426FA962cff2C9099835 – SMSVerification.sol
0xdD6a26f8971dF44698a2A3213512A5e3a0f97F61 – OprahBadge.sol, where you can get a badge by just requesting it for free
0xcF5A62987294fd2087252FD812443508528C52bF – BadgeReg.sol

@gavofyork registered on mainnet:

0x837A5e6f966945AbC67cB0B3f8ef62aE04Ed63ce – BadgeReg.sol
0x503b323dBe5457b7Ccc0Af9Bc246E5450B934E97 – SMSVerification.sol

The sms verification store got created when `isTestnet` (from the Redux state) was still `undefined`.

jacogr · 2016-11-29T11:16:17Z

I'm not sure I get what you mean by "blocked by MUI" - there is however already a "Badge" component ui/Badge (which is used by the TabBar - that can move that one to "Bubble", this can move to "Badge", everybody happy.)

There are a number of places where out components match the names of the MUI components, nothing wrong with that since we typically wrap and extend.

derhuerst · 2016-11-29T11:19:00Z

I'm not sure I get what you mean by "blocked by MUI" - there is however already a "Badge" component ui/Badge (which is used by the TabBar - that can move that one to "Bubble", this can move to "Badge", everybody happy.)

There are a number of places where out components match the names of the MUI components, nothing wrong with that since we typically wrap and extend.

The name "badge" is already "taken" by Material UI, so it might be unintuitive to have a component Badge that does something else than what most people would expect. It's only a minor impediment though.

jacogr · 2016-11-29T11:21:02Z

js/src/ui/Certifications/certifications.js

+
+import { hashToImageUrl } from '../../redux/providers/imagesReducer';
+
+const defaultIcon = '/api/content/371b226f700d8577fe849d7b2729bc2e4be8c06c38159fb880a6a0cc276af012';


No. We cannot do this. It assumes a certain network setup.

So the solution is to change hashToImageUrl to accept string hashes?

You can have a default icon pointing an Image file in Assets

jacogr · 2016-11-29T11:30:29Z

js/src/redux/providers/certifications/check-if-certified.js

+
+import ABI from '../../../contracts/abi/certifier.json';
+
+export default (api) => {


Any specific reason why we are in a "only a single function per file" mode here? As per previous PRs, we are just cluterring the directories and making multiple code imports, no reason no to have the functionality grouped. (I would put all files in this directory into one, easy to access, easy to see the whole picture)

Any specific reason why we are in a "only a single function per file" mode here?

This is just personal taste & being used to it. Will remove as far as I agree with the following point.

I would put all files in this directory into one, easy to access, easy to see the whole picture

On purpose, I stored middleware, actions and reducers in separate files. It underlines the whole style of using Redux by making it less tempting to intertwine actions & their reactions.

What about moving checkIfCertified and fetchCertifier into middleware.js?

If it makes sense, by all means.

This is not about personal taste, for the most part when you go into a component you cannot see if it was written by me, Nicolas, Jannis or Tomasz since it follows a consistent approach and way of doing things, where all adapted to. Would like to keep it that way and not leave personal preferences scattered along the codebase, I would prefer not looking at a component and knowing by the style who did it.

As long, as we don't mix things we shouldn't mix or break with best practices, I fully agree! :)

jacogr · 2016-11-29T11:32:36Z

js/src/views/Account/Header/header.js

@@ -67,6 +71,10 @@ export default class Header extends Component {
            <Balance
              account={ account }
              balance={ balance } />
+            <Certifications
+              certifications={ certifications }
+              dappsUrl={ api.dappsUrl }


Do we really want to pass this here? The api is on context and it is an Certification specific thing for rendering. Adds no real value from just having another prop for details available in the component already.

The idea is to have smart & dumb components or at least move in this direction, as it is considered best practice.

If Certifications is really only going to be used in this place, I will move it. Otherwise (e.g. when it's used in Accounts) it makes sense to keep Certifications dumb.

From what I can tell, certifications is a Redux state, thus we could connect Certifications to the state and make it dumb

@ngotchac Not sure I get what you mean, but "dumb"/presentational components are usually not aware of Redux.

What I meant is that you can connect your dumb Certifications component to the Redux store, so the Connector won't be dumb, but the presentation Component will be. It's just not to have to pass props to it all the way from Account which doesn't really care about the certifications per se

jacogr · 2016-11-29T11:34:07Z

js/src/views/Account/account.js

+      const { api } = this.context;
+      const { address } = nextProps.params;
+      this.setState({
+        verificationStore: new VerificationStore(api, address, isTestnet)


Any reason the store is not on the main class (like typically used elsewhere), but rather as a state component?

Is this related to https://github.com/ethcore/parity/pull/3665#discussion_r89991234 ?

No, it is just quite unintuitive having a state mechanism inside another state mechanism. Since you have a store (granted not easy here), you can get rid of state completely. So basically if you look what happens elsewhere, it is a case of -

static propTypes = { ... } store = new Store(...) render () { return ( <div>{ this.store.name }</div> ); }

So store is class-level, not within state. (In a perfect world these types of store components probably won't even have state, easy with new components, a process getting there with existing) So taking this state-within-state approach is probably due to a specific reason, just trying to understand why.

~~Sorry, thought you were talking about the badges. Just putting the store as a class prop didn't come to my mind, will move.~~

I can't instantiate VerificationStore right away, I need to wait for isTestnet having an actual (boolean) value. This is the problem I mentioned in Slack.

Still, I can move store from state into the instance itself. But then, the Reavt mechanisms of rerendering when changes happen won't work anymore, do they?

So why not waiting for isTestnet to be available before rendering the component ? :)

@ngotchac ~~So showing nothing/a loader, like discussed on Slack?~~

@ngotchac I put the verificationStore into <Account> to make it independent from the modal component. Back then, this was the first step of moving away from Redux.

I could wait for isTestnet to render the whole account page, but that's out of scope of this PR.

ngotchac · 2016-11-29T13:23:12Z

js/src/redux/providers/certifications/fetch-certifier.js

+
+import ABI from '../../../contracts/abi/badgereg.json';
+
+const address = '0xcF5A62987294fd2087252FD812443508528C52bF';


This should be in the register

outdated

ngotchac · 2016-11-29T13:24:27Z

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

+import badgeRegABI from '../../../contracts/abi/badgereg.json';
+import certifierABI from '../../../contracts/abi/certifier.json';
+
+const badgeRegAddress = '0xcF5A62987294fd2087252FD812443508528C52bF';


This should definitely be in the register, no ?

Had it on morden, but forgot it on ropsten.

ngotchac · 2016-11-29T13:25:29Z

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

+const knownCertifiers = [ 'smsverification' ];
+
+export const fetchCertifier = (api) => {
+  const registry = api.newContract(badgeRegABI, badgeRegAddress);


Why not having this in a contract JS file like tokenreg, githubhint, etc...?

ngotchac · 2016-11-29T13:32:09Z

js/src/views/Contract/contract.js

@@ -132,7 +140,9 @@ class Contract extends Component {
        <Page>
          <Header
            account={ account }
-            balance={ balance } />
+            balance={ balance }
+            certifications={ certificationsOfAccount }


Is this still needed?

jacogr · 2016-11-29T16:56:23Z

js/src/contracts/badgereg.js

+
+const ZERO = '0x0000000000000000000000000000000000000000000000000000000000000000';
+
+export default (api, registry) => {


Nothing wrong with the logic, but please follow the same style as the other contracts. This one is the inconsistent one, really don't need the extra mental overhead for "preferences".

jacogr · 2016-11-29T16:56:45Z

js/src/contracts/contracts.js

@@ -33,6 +34,7 @@ export default class Contracts {
    this._signaturereg = new SignatureReg(api, this._registry);
    this._tokenreg = new TokenReg(api, this._registry);
    this._githubhint = new GithubHint(api, this._registry);
+    this.badgeReg = BadgeReg(api, this._registry);


As per the above comment, fit in with what is there, please.

jacogr · 2016-11-29T16:58:11Z

js/src/redux/middleware.js

@@ -30,7 +31,8 @@ export default function (api) {
  const middleware = [
    settings.toMiddleware(),
    signer.toMiddleware(),
-    errors.toMiddleware()
+    errors.toMiddleware(),
+    certificationsMiddleware(api)


Any reason for not following the conventions, i.e. toMiddleware() as per the rest? (Took a long time to get all the middleware grouped and following a pattern after it was scattered all around.)

jacogr · 2016-11-29T16:59:04Z

js/src/views/Account/Header/header.js

@@ -67,6 +69,10 @@ export default class Header extends Component {
            <Balance
              account={ account }
              balance={ balance } />
+            <Certifications
+              account={ account.address }
+              dappsUrl={ api.dappsUrl }


Still don't believe dappsUrl should be passed in, not critical though, can live with it.

derhuerst · 2016-11-30T12:22:11Z

@gavofyork Feel free to complain about the visual style, as the technical problems are out of the way now.

~~BTW: Will also put the certfications in the address view, which is basically th place where they are the most interesting.~~ It is in there.

ngotchac · 2016-11-30T15:17:11Z

js/src/ui/Certifications/certifications.js

+}
+
+function mapStateToProps (state) {
+  return { certifications: state.certifications };


Quick note about Redux state : for containers like this, we should use this as mapStateToProps:

function mapStateToProps (_, initProps) { const { account } = this.props; return (state) => { const certification = state.certifications[account]; return { certification }; }; }

This is better because only the right certification Object is passed to the props, which means that the Component will not re-render when other certifications changes (which is the case if you pass the whole state.certifications Object).

coveralls · 2016-11-30T16:39:42Z

Coverage decreased (-0.03%) to 85.95% when pulling af18344 on jr-sms-badges into 79e7947 on master.

chevdor · 2017-09-17T14:30:58Z

In case someone needs to play on Ropsten, the info above is no longer valid:

To play with it on ropsten:

0x01e1a37118Fe3BeFd17C426FA962cff2C9099835 – SMSVerification.sol
0xdD6a26f8971dF44698a2A3213512A5e3a0f97F61 – OprahBadge.sol, where you can get a badge by just requesting it for free
0xcF5A62987294fd2087252FD812443508528C52bF – BadgeReg.sol

OprahBadge is at 0x72f4b7a806d8fd0a2f7d66c0CD1D71ea40Be5669
Additionnally, emailverification is at 0x4A99350b039068fD326a318C599Be2E09E657D4A

derhuerst added 7 commits November 28, 2016 19:14

sms verification: Certifications component

3fcf3e9

sms verification: actions & reducers for certifications

d777856

sms verification: put Certifications component into place

5c0cc16

sms verification: show certification icons

9019716

sms verification: show certification titles

69ffb60

sms verification: default icon for certifications

d06279a

sms verificaiton: lint issue 👕, fix testnet detection

c803626

The sms verification store got created when `isTestnet` (from the Redux state) was still `undefined`.

derhuerst added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Nov 29, 2016

jacogr reviewed Nov 29, 2016

View reviewed changes

derhuerst added 4 commits November 29, 2016 13:03

move certification helpers into middleware file

38461c1

connect Certifications to Redux

455b783

don't pass certifications as prop

fb6d6ab

move default certification icon into assets

64be315

ngotchac previously requested changes Nov 29, 2016

View reviewed changes

ngotchac reviewed Nov 29, 2016

View reviewed changes

derhuerst added 2 commits November 29, 2016 16:45

separate file for BadgeReg.sol

92fac31

don't pass certifications as prop

c6bcf5f

jacogr reviewed Nov 29, 2016

View reviewed changes

jacogr and others added 2 commits November 29, 2016 18:51

Fix import name

78e0bd3

make BadgeReg a class

66eb61b

derhuerst added 2 commits November 30, 2016 12:28

make certifications middleware a class

7efe6b4

merge master into jr-sms-badges

9cce91d

jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 30, 2016

ngotchac reviewed Nov 30, 2016

View reviewed changes

Certifications: pass in certifications of account

af18344

jacogr merged commit 784dcaf into master Nov 30, 2016

jacogr deleted the jr-sms-badges branch November 30, 2016 20:39

chevdor mentioned this pull request Sep 14, 2017

Badge display no longer works #6518

Closed


		import { hashToImageUrl } from '../../redux/providers/imagesReducer';

		const defaultIcon = '/api/content/371b226f700d8577fe849d7b2729bc2e4be8c06c38159fb880a6a0cc276af012';


		import ABI from '../../../contracts/abi/certifier.json';

		export default (api) => {


		import ABI from '../../../contracts/abi/badgereg.json';

		const address = '0xcF5A62987294fd2087252FD812443508528C52bF';


		const ZERO = '0x0000000000000000000000000000000000000000000000000000000000000000';

		export default (api, registry) => {

implement basic badges/certifications/flair #3665

implement basic badges/certifications/flair #3665

Conversation

derhuerst commented Nov 29, 2016 • edited Loading

jacogr commented Nov 29, 2016

derhuerst commented Nov 29, 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

jacogr Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

derhuerst Nov 29, 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

derhuerst Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

derhuerst Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derhuerst Nov 29, 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

jacogr Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

jacogr Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

derhuerst commented Nov 30, 2016 • edited Loading

Choose a reason for hiding this comment

coveralls commented Nov 30, 2016

chevdor commented Sep 17, 2017

derhuerst commented Nov 29, 2016 •

edited

Loading

derhuerst commented Nov 29, 2016 •

edited

Loading

jacogr Nov 29, 2016 •

edited

Loading

derhuerst Nov 29, 2016 •

edited

Loading

derhuerst Nov 29, 2016 •

edited

Loading

derhuerst Nov 29, 2016 •

edited

Loading

derhuerst Nov 29, 2016 •

edited

Loading

jacogr Nov 29, 2016 •

edited

Loading

jacogr Nov 29, 2016 •

edited

Loading

derhuerst commented Nov 30, 2016 •

edited

Loading