Add motion staking listeners #36

willm30 · 2023-03-21T10:00:15Z

This PR adds motion staking support to the block ingestor. It only handles that the event gets picked up and removed correctly. DB mutations are handled in a subsequent pr.

Test with this CDapp pr: port/270-stake-motion-saga (setup instructions therein)

Also, this PR contains new functionality in the event listener generator and event processor for removing listeners. Please test this out by:

installing and then immediately uninstalling the voting reputation extension
installing the voting rep extension; restarting the block ingestor; and then uninstalling the voting rep extn

You should see the following in the console:

willm30 · 2023-03-23T11:46:33Z

src/eventProcessor.ts

+  EXTENSION_INITIALISED = 'extensionInitialised',
+}
+
+export interface EventProcessorContext {


Allows the listeners to be removed by the extensionUninstalled handler that were generated in extensionInitialised

willm30 · 2023-03-23T11:47:46Z

src/handlers/motions/motionStaked/motionStaked.ts

+import { verbose } from '~utils';
+import { getMotionSide } from '../helpers';
+
+export default async (event: ContractEvent): Promise<void> => {


This is extended here and not the focus of this issue. We're just ensuring it picks up the event correctly

rdig

Seems all good.

rdig · 2023-03-26T23:31:48Z

src/utils/extensions/writeExtension.ts

+    eventProcessContext.removeListeners[colonyAddress]?.[
+      RemoveListenerKeys.EXTENSION_INITIALISED
+    ];


Uff. While I understand how this came about, it's really a mouth full to read :)

…ener

This reduces diffs in the subsequent pr

willm30 · 2023-03-27T16:59:07Z

src/context.ts

+
+const context: Context = { listenerRemovers: {} };
+
+export default context;


I have left off writing a getContext function like I did originally since objects are imported/exported by reference so it's not necessary in this context.

willm30 · 2023-03-27T17:20:26Z

src/utils/events/eventListeners/eventListenerGenerator.ts

 };

+/* Generate a unique key for the listenerRemover. */


When I say "unique", I'm assuming no client type will have two different events with the exact same signature. Therefore this combination of strings will effectively prevent unintended collisions.

If we kept a mapping between contract address and event signature, we could check for existing listeners before adding a new one. I experienced these collisions before in dev, because when restarted, the dev environment replays all the events that happened from block 1.

When restarting, memory is destroyed and recreated, no? We'd be starting with a fresh context object.

willm30 · 2023-03-27T17:22:30Z

@jakubcolony In this pr, I believe your concern is addressed re handling listeners. Now, the event generator listener can store the listeners in context directly, and I have a written a motion specific function that removes them and calls them (removeMotionListeners), which I call when the extension gets uninstalled.

Pls review when you get a chance.

willm30 · 2023-03-27T17:23:41Z

@rdig I pushed some changes to this after you approved to take into account Jakub's suggestion of reducing the points of contact with the context object in the code base. It all stills works but if you'd like to have another glance, that'd be great.

These commits specifically:

51a0acc
519fdd0
1fe0463

rdig

All good from my side

jakubcolony

Looks much better already but I'm asking for one more round of simplification and a few changes.

One issue is that this is too specific for motions, and without jumping into the future, it doesn't remove the existing ExtensionInitialised listeners when an extension gets removed. Could some changes be made so that it removes all the listeners for a particular contract address when it's no longer relevant?

I think it's important for a new functionality like that to be as isolated as possible and only communicate with the "outside" world by some explicitly exposed methods that can hide all the complexity. Think of it as a class that could do all the work privately and only have a few public methods, such as registerRemover, removeListener, etc. Ideally, all the code related to removing listeners could form a little module within the ingestor.

jakubcolony · 2023-04-06T09:53:45Z

src/context.ts

@@ -0,0 +1,11 @@
+export type ListenerRemover = () => void;


I still can't see the benefit of introducing this concept. It just seems like a place that may grow to contain many unrelated things. Why not just keep it simple and export the functionality directly?

Refactoring this would also allow you to collocate all the code related to removing listeners together, this is definitely improved since the first iteration but still feels like it's in too many places (context, utils, event listeners)

jakubcolony · 2023-04-06T10:16:35Z

src/utils/motions.ts

@@ -53,3 +62,34 @@ export const writeMintTokensMotionToDB = async (
    },
  });
 };
+
+const getMotionListenerRemovers = (


Should they be motion specific, or contract address specific? If an extension is uninstalled, a colony deleted (for example, can't happen right now), etc., we probably want to remove all the listeners for that address, not just motions?
This would be another good reason to extract the entire listener removers into their own little module

jakubcolony · 2023-04-06T10:18:31Z

src/utils/events/eventListeners/eventListenerGenerator.ts

 };

+/* Generate a unique key for the listenerRemover. */


If we kept a mapping between contract address and event signature, we could check for existing listeners before adding a new one. I experienced these collisions before in dev, because when restarted, the dev environment replays all the events that happened from block 1.

jakubcolony · 2023-04-06T10:22:57Z

src/utils/events/eventListeners/eventListenerGenerator.ts

+    const listenerRemoverKey = generateListenerRemoverKey(
+      contractAddress,
+      clientType,
+      eventSignature,
+    );
+    ctx.listenerRemovers[listenerRemoverKey] = listenerRemover;


I can imagine there could be a registerListener method that would accept some parameters, then generate the key and add it to the "cache" outside of this function

How could this logic be moved outside of the function? We need a reference to the exact event handler we're removing. Previously I accessed this by returning it from the eventListenerGenerator but based on your feedback have moved it inside.

What I imagined was something like this:

// listenerRemover.ts const removers = {}; // don't need context, this can be internal to the module export const saveRemover = (contractAddress, eventSignature, listenerRemover) => { const key = generateListenerRemoverKey(...) removers[key] = listenerRemover; }

Then your code in the generator shortens to declaring the remover and calling the saveRemover function. There's no need for the eventListenerGenerator to know about how the removers are stored/accessed.

That's where my analogy to classes came from; you hide as much of the internals as possible, and only expose the public interface (saveRemover and other functions)

willm30 · 2023-04-07T08:32:45Z

@jakubcolony I don't in principle disagree with your feedback, but I would point out that there are many block-ingestor concepts that could have been implemented via classes, and haven't been. Given that, I feel like:

a) my proposed changes are consistent with the style of the repo, and
b) I would benefit from further reading to get myself up to speed with the pattern you're suggesting before implementing it (recommended reading welcome).

I'd also ask you to apply the following test taken from the google engineering practices doc:

In general, reviewers should favor approving a CL once it is in a state where it definitely improves the overall code health of the system being worked on, even if the CL isn't perfect.

I feel like your suggestions could be the subject of a refactoring issue as/when we introduce a new use case for listener removers (currently motions are the only one, right?). Else we risk running the gauntlet of YAGNI / premature abstraction.

I think the listener-remover changes made here are:

consistent with the repo style, and
improve the health of the repo

They may not be perfect, but I'd prefer we handle changes in a specific refactoring issue if/when it becomes apparent they're needed.

(Btw, I do appreciate your comments! The above is more a discussion of process rather than substance.)

jakubcolony · 2023-04-12T07:59:03Z

@willm30 Sorry, my comment might have been written badly. What I meant is collocating the related code together (whatever style you write it; functional as you pointed out is the prevalent one) and gave an OOP class concept as one nicely depicting that - my bad, should've made it clearer. My main point was to keep it together, rather than having the logic spanning across multiple files.

I feel like your suggestions could be the subject of a refactoring issue as/when we introduce a new use case for listener removers (currently motions are the only one, right?). Else we risk running the gauntlet of YAGNI / premature abstraction.

There's the ExtensionInitialised event listener added for each installed extension that should also be removed. There could be a separate issue for that, but I think keeping that in mind might help you refactor this easier (having a separate "module" for removing listeners that would hold all the logic). Tying it closely to motions makes it pretty difficult to refactor down the line.

willm30 · 2023-04-12T09:43:16Z

@jakubcolony Thanks for clarifying. Please review 13f7577 when you get a chance.

Removing the extension initialised listener can be handled here: #49

It should be straightforward enough and has been kept in mind when writing the functionality inside listenerRemover.ts.

willm30 · 2023-04-12T09:46:27Z

src/utils/events/eventListeners/listenerRemover.ts

+const getRemovers = (keys: string[]): ListenerRemover[] => {
+  const listenerRemovers: ListenerRemover[] = [];
+
+  keys.forEach((key) => {


I could map here but this is slightly safer because it guards against the scenario where removers[key] returns undefined

(and therefore avoids needing to call removeListeners() as removeListeners?.())

Very much a just in case, not expecting it to return undefined

jakubcolony

Thanks for addressing the feedback, great work! 👌

Add: motion id to db when motion created

b22e305

willm30 self-assigned this Mar 21, 2023

willm30 mentioned this pull request Mar 21, 2023

Wire in Stake motion saga JoinColony/colonyCDapp#335

Merged

willm30 commented Mar 23, 2023

View reviewed changes

willm30 force-pushed the add/motion-staking-support branch 3 times, most recently from da697e4 to 74eb9a2 Compare March 23, 2023 12:36

willm30 mentioned this pull request Mar 23, 2023

Update UI after staking a motion JoinColony/colonyCDapp#338

Merged

willm30 marked this pull request as ready for review March 23, 2023 13:40

willm30 changed the title ~~Add motion staking support~~ Add motion staking listeners Mar 23, 2023

willm30 requested a review from a team March 23, 2023 13:41

willm30 changed the base branch from add/motion-data-to-db to lint/port-motions March 23, 2023 13:45

This was referenced Mar 23, 2023

Add motion data to db when motion created #35

Closed

Add motion staking support #18

Closed

rdig approved these changes Mar 26, 2023

View reviewed changes

Base automatically changed from lint/port-motions to port/motions March 27, 2023 09:25

willm30 added 9 commits March 27, 2023 14:05

feat: create and export a context object

51a0acc

feat: create wrapper for motions listeners, and add stake motion list…

272e1f2

…ener

refactor: replace old motion listener with new motion listener

33df692

refactor: move motion types to own file

26c1b63

feat: add motionStaked handler to event processor

4132222

feat: add 'removeListener' functionality to event listener generator

519fdd0

refactor: addMotionEventListener to save remover in context

5416056

refactor: move motionStaked to own directory

74944c2

This reduces diffs in the subsequent pr

feat: remove motion listeners when voting rep uninstalled

1fe0463

willm30 force-pushed the add/motion-staking-support branch from 74eb9a2 to 1fe0463 Compare March 27, 2023 16:57

willm30 commented Mar 27, 2023

View reviewed changes

willm30 requested a review from rdig March 27, 2023 17:22

rdig approved these changes Mar 29, 2023

View reviewed changes

willm30 requested a review from jakubcolony April 5, 2023 21:24

willm30 mentioned this pull request Apr 5, 2023

Add db mutations for motion staking #37

Merged

jakubcolony requested changes Apr 6, 2023

View reviewed changes

willm30 mentioned this pull request Apr 12, 2023

Remove ExtensionInitialised listener when extension is uninstalled #49

Closed

refactor: move remover logic to own file

13f7577

willm30 requested a review from jakubcolony April 12, 2023 09:43

willm30 commented Apr 12, 2023

View reviewed changes

jakubcolony approved these changes Apr 12, 2023

View reviewed changes

willm30 merged commit 374bc4a into port/motions Apr 12, 2023

willm30 deleted the add/motion-staking-support branch April 12, 2023 14:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add motion staking listeners #36

Add motion staking listeners #36

willm30 commented Mar 21, 2023 •

edited

Loading

willm30 Mar 23, 2023

willm30 Mar 23, 2023

rdig left a comment

rdig Mar 26, 2023

willm30 Mar 27, 2023

willm30 Mar 27, 2023

jakubcolony Apr 6, 2023

willm30 Apr 7, 2023

willm30 commented Mar 27, 2023

willm30 commented Mar 27, 2023 •

edited

Loading

rdig left a comment

jakubcolony left a comment

jakubcolony Apr 6, 2023

jakubcolony Apr 6, 2023

jakubcolony Apr 6, 2023

jakubcolony Apr 6, 2023

jakubcolony Apr 6, 2023

willm30 Apr 7, 2023

jakubcolony Apr 12, 2023 •

edited

Loading

willm30 commented Apr 7, 2023

jakubcolony commented Apr 12, 2023

willm30 commented Apr 12, 2023

willm30 Apr 12, 2023 •

edited

Loading

willm30 Apr 12, 2023

jakubcolony left a comment


		const context: Context = { listenerRemovers: {} };

		export default context;

Add motion staking listeners #36

Add motion staking listeners #36

Conversation

willm30 commented Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdig left a 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

willm30 commented Mar 27, 2023

willm30 commented Mar 27, 2023 • edited Loading

rdig left a comment

Choose a reason for hiding this comment

jakubcolony left a 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

jakubcolony Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

willm30 commented Apr 7, 2023

jakubcolony commented Apr 12, 2023

willm30 commented Apr 12, 2023

willm30 Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakubcolony left a comment

Choose a reason for hiding this comment

willm30 commented Mar 21, 2023 •

edited

Loading

willm30 commented Mar 27, 2023 •

edited

Loading

jakubcolony Apr 12, 2023 •

edited

Loading

willm30 Apr 12, 2023 •

edited

Loading