Major core rewrite! #431

taoeffect · 2018-06-20T05:36:58Z

Removed dependencies

protobufjs
objection
knex
sqlite3
superagent (pure fetch-based codebase now)

`shared/events.js` rewrite

New GIMessage core class
Now virtually everything moved under model/contracts/*
SBP is now used to contruct messages

Database improvements

Now Group Income contract chains are designed to work purely
with key-value stores
backend/database.js and model/database.js are now virtually
identical and in the future can be made to use the same library

Misc. improvements

Simplified backend/routes.js API
Re-added explicit .js to imports (please always include it!)
Replaced all Vue.events with SBP 'okTurtles.events/*'
Various code improvements
Backend tests now use same pubsub and backend API code as frontend
Deleted lots of classes and converted more parts of the app to SBP
Greatly simplified handleEvent action in state.js
Changed okTurtles.events API to match Vue.events behavior

### Removed dependencies - protobufjs - objection - sqlite3 - superagent (pure `fetch`-based codebase now) ### `shared/events.js` rewrite - New `GIMessage` core class - Now virtually everything moved under `model/contracts/*` - SBP is now used to contruct messages ### Database improvements - Now Group Income contract chains are is designed to work purely with key-value stores - `backend/database.js` and `model/database.js` are now virtually identical and in the future can be made to use the same library ### Misc. improvements - Simplified `backend/routes.js` API - Re-added explicit `.js` to imports (please always include it!) - Replaced all `Vue.events` with SBP `'okTurtles.events/*' - Various code improvements - Backend tests now use same pubsub and backend API code as frontend - Deleted lots of classes and converted more parts of the app to SBP - Greatly simplified `handleEvent` action in `state.js` - Changed `okTurtles.events` API to match `Vue.events` behavior

taoeffect · 2018-06-22T03:08:33Z

Copying from Slack:

- Now passes all `test/backend.js` tests
- Passes most `test/frontend.js` tests
- Is ready for your review
- Could use @aniko's help for figuring out why the frontend tests aren't passing ... and it might be because of some of the proposal stuff anyway, which has been rewritten in @aniko's PR
- So therefore I think it should be safe for @aniko to begin rebasing off of it, since she needs to change some of the proposals stuff anyway, and is working on tests for that, and in the process can maybe figure out or fix the issue with mine

sandrina-p · 2018-06-24T11:20:20Z

frontend/simple/controller/backend.js

+  var idx = subscriptions.indexOf(contractID)
+  var method = data.add ? 'sub' : 'unsub'
+  if ((data.add && idx > -1) || (data.remove && idx === -1)) {
+    return // if already subscribed or already unsubscribed


Usually a function should always return something, even if false only, that I believe it's what's intended here. return false

This function doesn't return anything, so returning undefined is perfectly fine

sandrina-p · 2018-06-24T14:06:21Z

frontend/simple/model/contracts/group.js

+        if (state.proposals[data.proposalHash]) {
+          state.proposals[data.proposalHash].for.push(data.username)
+          let threshold = Math.ceil(state.proposals[data.proposalHash].threshold * Object.keys(state.profiles).length)
+          if (state.proposals[data.proposalHash].for.length >= threshold) {


state.proposals[data.proposalHash] is used 4 times here. what about having a variable?

I'm avoiding messing with the old proposals code as I hope @hubudibu will be able to rewrite it and improve upon it, so my messing with it right now probably wouldn't be worthwhile

sandrina-p · 2018-06-24T15:45:29Z

shared/domains/okTurtles/events/index.js

  'okTurtles.events/on': function (event: string, handler: Function) {
-    sbp('okTurtles.data/add', `events/${event}/listeners`, handler)
+    sbp('okTurtles.data/add', listenKey(event), handler)


Cool work here! The new logs on the console can be helpful!

I played a little with sbp() locally during this CR and some questions came to my mind:

Does sbp have a naming conventions for the selectors? I saw selectores in camelCase and kebab-case, (backend/publishLogEntry vs gi/contract/create-action) and I believe we should sticky with one, similar to what Vue did with events names (kebab-case).

Do we have guidelines to decide the scope of a selector? backend, gi, okturtles, namespace, state, etc... It's not hard to give the same name twice. How do you plan to avoid this besides using an SBP logging facility?

I registered my first sbp() using 'sbp/selectors/register' and 'okTurtles.events/on' with ease!

// GroupDashboard.vue created () { sbp('sbp/selectors/register', { 'test/selector-1': (origin) => console.log('My first selector from ' + origin) }) sbp('okTurtles.events/on', 'hello', (origin) => console.log('My first hello emit from ' + origin) ) }

// Voting.vue handleVoteFor () { sbp('test/selector-1', 'selector-voting') sbp('okTurtles.events/emit', 'hello', 'emit-voting') // ... },

And both logged

> My first selector from selector-voting > My first hello emit from emit-voting

So, it looks like they both do the same but with different usages. Can you tell/explain me the core difference between both and when should I use one or another?

Yes indeed they look somewhat similar but the semantics are rather different, for example:

You can emit an event that has no listeners but you cannot call a selector that hasn't been registered

A single event can have multiple listeners but a selector can have only one function registered for it

When you emit an event there is no return value, but when you can a selector there is

etc.

Generally speaking, for all Vue related things you should probably use events only. Selectors should be registered only for things that can be conceptualized as services. In this way events are a bit more "light weight" than selectors.

By separating events and selectors we also have the ability to do things like add SBP filters just for events that are emitted, so for example, if we wanted to re-integrate the Vue.js Chrome debugger extension with our events system, so that okTurtles.events appear in the Vue events debugger, we could add an SBP filter to emit Vue events for every okTurtles.events that were emitted, while ensuring that SBP calls do not appear in that list.

Does sbp have a naming conventions for the selectors? I saw selectores in camelCase and kebab-case, (backend/publishLogEntry vs gi/contract/create-action) and I believe we should sticky with one, similar to what Vue did with events names (kebab-case).

I will think on this. I agree it's important but it's also a decision that requires a lot of thought and I wanted to get this PR in quickly.

Do we have guidelines to decide the scope of a selector? backend, gi, okturtles, namespace, state, etc... It's not hard to give the same name twice

So that would be called the "domain" of a selectors. And no, at the moment we haven't figured out good guidelines for that, besides making sure to only register domains for clearly defined services, and avoid name collisions.

I am considering prefixing backend etc. under the gi domain, or gi.frontend domain, but this is something that I'll need to give more thought to.

Thank you for the extended clarification. It would be cool to add the emit vs register sbp usage somewhere on our docs :)

sandrina-p · 2018-06-24T15:49:01Z

shared/sbp.js

    for (const selector in sels) {
      // TODO: debug log (using an SBP logging facility) if we're already registered
      if (!selectors[selector]) {
        selectors[selector] = sels[selector]
+        registered.push(selector)
      }


Can we add a quick solution to warn when a selector already exists

} else { console.warn(`The selector ${selector} already exists`) }

sandrina-p · 2018-06-24T16:04:16Z

shared/domains/okTurtles/data/index.js

-      targetArray.push(data)
+    const array = _store.get(key)
+    if (array) {
+      array.push(data)


Do we need to verify if data already exists on array value, or can it have the same data twice?

var array = store.get('bar') // ['foo', 'foo2'] array.push('foo2'); console.log(store.get('bar')); // ['foo', 'foo2', 'foo2'] // Same data twice!

Here it's fine to have the same data twice for performance reasons. If it's an issue for the caller they can be the ones to check for duplicate data.

hubudibu · 2018-06-28T18:49:15Z

frontend/simple/controller/backend.js

+  } else {
+    subscriptions.splice(idx, 1)
+  }
+  var res = await primus[method](contractID)


this seems like a risky few lines! how about doing & awaiting the primus sub/unsub and updating the subscriptions array after that?

hubudibu · 2018-06-29T21:42:47Z

shared/events.js

+  return blake32Hash(data)
+}
+
+export class GIMessage {


can we rename this file to GIMessage or something alike? events doesn't really say much about this file any more :)

hubudibu · 2018-06-30T17:38:53Z

frontend/simple/controller/backend.js

+}
+
+var subscriptions = []
+var primus


I’ve been thinking why it was so hard for me to process this file and I think we can make it much easier to understand with a few naming changes.

primus for me means a) the god of transformers b) a tool that we use in another file. a name saying what it does here would be more helpful, eg. socket or, more specifically, contractSocket, as this seems to be for syncing contracts?

similarly, I had an aha moment when I realized that subscriptions is storing contracts and that’s what is referred to as rooms at other places, so contractSubscriptions or contractRooms or something alike could be more helpful?

hubudibu · 2018-07-02T14:51:25Z

frontend/simple/model/utils.js

+    // and when that GIMessage is received and is applied as a mutation
+    vuexModule.mutations[name] = function (state, data) {
+      validate(data.data)
+      vuex.mutation(state, data)


the naming here is a bit misleading, I just spent a considerable amount of time trying to figure out what's this secret vuex feature that somehow constructs mutations :) where this is just a key in our contract structure. I would suggest using a name describing the concept it's for, and not the actual tool name. like contractStore or at least forVuex or something?

in general, I don't find this utility function that useful, it seems like most of what it does is make the contracts look significantly different from standard vuex modules with all these transformations, thus making them harder to read & understand.

Ok, using vuexModuleConfig, sound ok?

sounds much better!

hubudibu · 2018-07-02T14:53:30Z

frontend/simple/model/utils.js

+
+    // NOTE: must check explicitely if 'constructor' is true
+    //       because that key is defined by default on objects as some function...
+    if (constructor === true) {


I would suggest using a different name here, like isConstructor, as constructor is like a 'reserved word' in js land, and as the comment already notes, we are in some cases overwriting existing functions this way

hubudibu · 2018-07-02T15:21:17Z

frontend/simple/model/state.js

@@ -258,79 +277,47 @@ const actions = {
  // mirrors `handleEvent` in backend/server.js
  async handleEvent (


this comment explaining what this method does seems no longer to be true

hubudibu · 2018-08-02T19:10:12Z

test fix 1 in test/frontend.js line 429:

      await n
        // .goto(page('mailbox'))
        // TODO: navigation gets redirected on login guard but nav click doesn't?
        // we might have logged in state problems
        .wait(elT('mailboxLink'))
        .click(elT('mailboxLink'))

also, what's the workflow if I want to push to someone else's PR?

ktamas77 · 2018-08-02T19:10:04Z

backend/auth.js

@@ -21,7 +21,7 @@ exports.register = function (
        if (scheme !== 'gi') return reply(Boom.badRequest('Bad authentication'))

        try {
-          json = JSON.parse(b642str(json))
+          json = JSON.parse(b64ToStr(json))


this is definitely a way more fortunate naming convention for this function.

... that's a book. Can you be more specific?

Oh, I misread your comment, thanks!

I was just confirming that b64ToStr() is way better (less cryptic) than b642str().
Although the best could be base64ToString() - the less it makes me think, the less i have to decipher mnemonics in my brain the easier it is to understand.

ktamas77 · 2018-08-02T19:12:02Z

backend/database.js

 // delete the test database if it exists
-!production && fs.existsSync('test.db') && fs.unlinkSync('test.db')


glad to see this go too! =)

hubudibu · 2018-08-03T00:37:22Z

next test fix step: we didn't clean up properly after logout, so the message contracts got borked together. next step here: hubudibu@457cfa2

vijayee · 2018-08-03T03:31:46Z

In the template of GroupDashboard.vue on line 11 there needs to be conditional rendering that checks for the current group state before trying to render its properties

 <div class="column container gi-main" v-if="currentGroupState" >
        <div class="gi-header">
          <div class='is-pulled-right'>
            <groups-min-income :group="currentGroupState" />
          </div>

          <h1 class="title is-1" data-test="groupName">
            {{ currentGroupState.groupName }}
          </h1>
          <p data-test="sharedValues">
            {{ currentGroupState.sharedValues }}
          </p>
        </div>

        <dashboard-section title="Proposals">
          <proposals :proposals="currentGroupState.proposals" />
        </dashboard-section>

        <dashboard-section title="Members" data-test="groupMembers">
          <group-members />
        </dashboard-section>

        <dashboard-section title="July Overview">
          <progress-overview />
        </dashboard-section>

        <dashboard-section title="Support History">
          <support-history :history="[1.2, 1, .85, .95, 1.05, .35]" />
        </dashboard-section>

        <dashboard-section title="Group Settings">
          <group-settings :group="currentGroupState" />
        </dashboard-section>
      </div>
    </div>

taoeffect requested review from hubudibu and sandrina-p June 20, 2018 05:36

taoeffect changed the title ~~[WIP] Major core rewrite!~~ Major core rewrite! Jun 22, 2018

Now all backend and most frontend tests are passing!

d13fa9e

taoeffect force-pushed the sbpify branch from 5ed73cb to d13fa9e Compare June 22, 2018 02:52

Added 'initialState' key to GenerateContract + more giLodash goodness

d70d0ab

taoeffect force-pushed the sbpify branch from 5cdd54a to d70d0ab Compare June 22, 2018 06:13

improved okTurtles.events implementation

7f587bd

sandrina-p reviewed Jun 24, 2018

View reviewed changes

hubudibu reviewed Jul 2, 2018

View reviewed changes

ktamas77 reviewed Aug 2, 2018

View reviewed changes

taoeffect mentioned this pull request Aug 2, 2018

Figure out why goto(page('mailbox')) broke #440

Closed

Incorporated Aniko's PR review (thanks @hubudibu!)

96a4a7d

taoeffect added 3 commits August 2, 2018 16:39

updated nightmare to 3 and improved frontend tests

39b1382

new fancy stuff is at the same place aniko & co are at :)

a509d8d

added logouts for logins

77a9bfa

taoeffect added 3 commits August 2, 2018 19:22

fixed a test

148ba8e

fixed proposal voting tests! all tests passing!

8cbbdef

remove knex dependency

67f40f4

hubudibu merged commit f561818 into okTurtles:master Aug 3, 2018

This was referenced Aug 6, 2018

Refactor interaction with chain & everything else to SBP #295

Closed

Add write guards to accessors on Hashable and HashableEntry #226

Closed

Modifying group profile should only send one txn #283

Closed

taoeffect mentioned this pull request Aug 30, 2018

GIMessage hashes are insecure and non deterministic #479

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Major core rewrite! #431

Major core rewrite! #431

taoeffect commented Jun 20, 2018 •

edited

Loading

taoeffect commented Jun 22, 2018

sandrina-p Jun 24, 2018

taoeffect Jun 27, 2018

sandrina-p Jun 24, 2018

taoeffect Jun 27, 2018

sandrina-p Jun 24, 2018

taoeffect Jun 24, 2018

sandrina-p Jun 26, 2018

sandrina-p Jun 24, 2018

sandrina-p Jun 24, 2018

taoeffect Jun 24, 2018

sandrina-p Jun 26, 2018

hubudibu Jun 28, 2018

hubudibu Jun 29, 2018

hubudibu Jun 30, 2018

hubudibu Jul 2, 2018

taoeffect Aug 2, 2018

hubudibu Aug 2, 2018

hubudibu Jul 2, 2018

hubudibu Jul 2, 2018

hubudibu commented Aug 2, 2018

ktamas77 Aug 2, 2018

taoeffect Aug 2, 2018

taoeffect Aug 2, 2018

ktamas77 Aug 3, 2018

ktamas77 Aug 2, 2018

hubudibu commented Aug 3, 2018

vijayee commented Aug 3, 2018

		@@ -258,79 +277,47 @@ const actions = {
		// mirrors `handleEvent` in backend/server.js
		async handleEvent (

		// delete the test database if it exists
		!production && fs.existsSync('test.db') && fs.unlinkSync('test.db')

Major core rewrite! #431

Major core rewrite! #431

Conversation

taoeffect commented Jun 20, 2018 • edited Loading

Removed dependencies

shared/events.js rewrite

Database improvements

Misc. improvements

taoeffect commented Jun 22, 2018

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

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

hubudibu commented Aug 2, 2018

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

hubudibu commented Aug 3, 2018

vijayee commented Aug 3, 2018

taoeffect commented Jun 20, 2018 •

edited

Loading

`shared/events.js` rewrite