Added lifecycle hooks to solver table by tamir-cow · Pull Request #85 · cowprotocol/cms

tamir-cow · 2026-02-18T08:51:55Z

As part of our recent efforts to improve data integrity in the CMS we are adding event hooks to certain actions in the cms that update other fields, this way we ensure that data is always up to date in realtime unlike now when fields are not updated because of manual mistakes or things changing in one table but not reflected in another.

In this pr we add some simple lifecycles to the solver table:
before create or update operations on the solver table:

Update active networks field (list of chains solver is live on) of solver. Checked by referncing solver network table.
Update service fee enabled status field based on bonding pool solver is on and date he joined it (CoW, colocated, full).

To test this change:

Deploy cms locally
Create solver bonding pool entry
Create solver network entry
Create / update solver table
Check isServiceFeeEnabled field
Check activeNetworks

anxolin · 2026-02-18T11:02:46Z

Hello Tamir. Could you try to add a bit more context in the description of the PR. The description is a bit hard to read, and is good to add a section explaining how to test this (steps), this makes the review easier and reduce the chance of misunderstanding the scope and goal of the PR

anxolin

The description of the PR is way better now. Thanks for applying the changes

anxolin · 2026-02-18T11:54:05Z

src/api/solver/content-types/solver/lifecycles.ts

+}
+
+// Helper function to calculate months difference between two dates
+function getMonthsDifference(startDate: Date, endDate: Date): number {


This belong to a utils file, in case you need it in another part of the project you don't want to add a dependency to this file

anxolin · 2026-02-18T11:56:01Z

src/api/solver/content-types/solver/lifecycles.ts

+  // For create operation, we handle it in afterCreate since we need the ID
+}
+
+async function calculateServiceFeeEnabled(solver: Solver, data: SolverData): Promise<void> {


why is this called calculate, but returns nothing?

anxolin · 2026-02-18T11:57:03Z

src/api/solver/content-types/solver/lifecycles.ts

+  // For create operation, we handle it in afterCreate since we need the ID
+}
+
+async function calculateServiceFeeEnabled(solver: Solver, data: SolverData): Promise<void> {


Would be positive to add some minmial JsDoc to explain what is the responsability of the function

anxolin · 2026-02-18T11:57:21Z

src/api/solver/content-types/solver/lifecycles.ts

+}
+
+async function calculateServiceFeeEnabled(solver: Solver, data: SolverData): Promise<void> {
+  data.isServiceFeeEnabled = false;


this is mutating the parameter, is this desired?

Yes, if its not set to true it needs to be false by default

anxolin · 2026-02-18T11:58:31Z

src/api/solver/content-types/solver/lifecycles.ts

+      const joinedDate = new Date(bondingPool.joinedOn);
+      const monthsDifference = getMonthsDifference(joinedDate, currentDate);
+
+      if (bondingPool.name === "CoW" && solver.isColocated === "No") {


Why name CoW, its hard to me to reason about this logic

This is the bonding pool name in the referenced table (an existing entry)

This logic seems to fragile to me. So you are following an undocumented naming convention, that is hidden in a method of this lifecycle.

Also, this method suffers from the same things I mentioned here #85 (comment)

called calculate, but it mutates

does too many things, should rely on somethign th

has a lot of logic that for me is hard to tell if this is what you intended to do. You have bussiness logic mixed there that I can't verify (hardcoded 3 months, skip if joinedOn is missing, ans different rules for collocated cow and for others). How can I tell what I need to verify?

src/api/solver/content-types/solver/lifecycles.ts

anxolin · 2026-02-18T12:01:28Z

src/api/solver/content-types/solver/lifecycles.ts

+      );
+
+      if (solver) {
+        await calculateServiceFeeEnabled(solver as Solver, solverData);


it feels like you want to update something and first you want to do some calculations. If that's the case, is better to make a pure function to decide the new value and then do the update

This function relies on side effects and does too many things. Follow SOLID single responsability principle "https://www.digitalocean.com/community/conceptual-articles/s-o-l-i-d-the-first-five-principles-of-object-oriented-design#single-responsibility-principle

All comments showed outdated code and I couldn't find the original function.

When you address a comment and code changes a lot so GH can placed a discussion in the right place of the new code, is good you include a link to the commit you pushed to address the comment.

Ideally, you move part of calculateActiveNetworks to a method getActiveNetworks(solver.solver_networks) this way your method is way more readable.

Also, it doesn't calculate, it updates the solver. Calculate implies is READ ONLY, when updates implies it MUTATES which is what you did

So it could be something like:

async function updateActiveNetworks(solver: Solver, data: SolverData): Promise<void> { const activeNetworks = getActiveNetworks(solver.solver_networks) data.hasActiveNetworks = activeNetworks.length > 0; data.activeNetworks = activeNetworks ? activeNetworks.map(network => network..name) : ''; }

Not only this is simpler and more readable than the current code, but also, if you remove a network, you won't get the value you expect because of how you return early (you don't update the data if there's no network info).

Original code:

async function calculateActiveNetworks(solver: Solver, data: SolverData): Promise<void> { if (!solver.solver_networks) { data.activeNetworks = []; data.hasActiveNetworks = false; return; } // Filter active networks and extract their names const activeNetworkNames = solver.solver_networks .filter(network => network.active) .map(network => network.network?.name) .filter(Boolean) as string[]; // Remove any undefined values data.activeNetworks = activeNetworkNames; data.hasActiveNetworks = activeNetworkNames.length > 0; }

bram-vdberg · 2026-02-25T05:15:26Z

src/api/solver/content-types/solver/lifecycles.ts

+    const joinedDate = new Date(bondingPool.joinedOn);
+    const monthsDifference = getMonthsDifference(joinedDate, currentDate);
+
+    if (bondingPool.name === "CoW" && solver.isColocated === "No") {


I don't think we should do this in a lifecycle. Having this logic in the lifecycle means that the BL for updating the CMS is split into two different places, both in Dagster and the CMS itself. That adds unnecessary complexity and makes it harder to maintain in the future. I think it would be better to do this in Dagster and keep a separation where data is transformed and updated through Dagster and these lifecycles handle simple aggregation.

bram-vdberg · 2026-02-25T05:20:23Z

src/api/solver/content-types/solver/lifecycles.ts

+export default {
+  async beforeCreate(event) {
+    try {
+      await updateActiveNetworks(event);


Shouldn't this be done after updating or creating?

bram-vdberg · 2026-02-25T05:21:35Z

src/api/solver/content-types/solver/lifecycles.ts

+
+  async beforeUpdate(event) {
+    try {
+      await updateActiveNetworks(event);


Since these updates and creates will be done through Dagster, what's the advantage of processing this through the lifecycle vs adding this update as a step in the same Dagster job that will do the update that triggers this hook?

anxolin · 2026-02-25T11:11:19Z

src/api/solver/content-types/solver/lifecycles.ts

+ * Determines if a solver is eligible for service fees based on bonding pool criteria.
+ * Pure function that checks eligibility based on time thresholds:
+ * - 6+ months for non-colocated CoW pools
+ * - 3+ months for colocated pools


Just curious, is this comming from EIP, if so. Could you add it?

anxolin · 2026-02-25T11:17:01Z

src/api/solver/content-types/solver/lifecycles.ts

+ * - 6+ months for non-colocated CoW pools
+ * - 3+ months for colocated pools
+ * @param solver The solver entity to check
+ * @returns Boolean indicating if the solver is eligible for service fees


TBH, i don't know what is service fee exactly. I assume this is not protocol fee related.

To avoid these things, if you give more context in the PR you avoid people that is not familiar with your current work to get a proper idea

anxolin · 2026-02-25T11:19:52Z

src/api/solver/content-types/solver/lifecycles.ts

+      );
+
+      if (solver) {
+        await calculateServiceFeeEnabled(solver as Solver, solverData);


All comments showed outdated code and I couldn't find the original function.

When you address a comment and code changes a lot so GH can placed a discussion in the right place of the new code, is good you include a link to the commit you pushed to address the comment.

anxolin · 2026-02-25T11:30:57Z

src/api/solver/content-types/solver/lifecycles.ts

+      );
+
+      if (solver) {
+        await calculateServiceFeeEnabled(solver as Solver, solverData);


Ideally, you move part of calculateActiveNetworks to a method getActiveNetworks(solver.solver_networks) this way your method is way more readable.

Also, it doesn't calculate, it updates the solver. Calculate implies is READ ONLY, when updates implies it MUTATES which is what you did

So it could be something like:

async function updateActiveNetworks(solver: Solver, data: SolverData): Promise<void> { const activeNetworks = getActiveNetworks(solver.solver_networks) data.hasActiveNetworks = activeNetworks.length > 0; data.activeNetworks = activeNetworks ? activeNetworks.map(network => network..name) : ''; }

Not only this is simpler and more readable than the current code, but also, if you remove a network, you won't get the value you expect because of how you return early (you don't update the data if there's no network info).

Original code:

async function calculateActiveNetworks(solver: Solver, data: SolverData): Promise<void> { if (!solver.solver_networks) { data.activeNetworks = []; data.hasActiveNetworks = false; return; } // Filter active networks and extract their names const activeNetworkNames = solver.solver_networks .filter(network => network.active) .map(network => network.network?.name) .filter(Boolean) as string[]; // Remove any undefined values data.activeNetworks = activeNetworkNames; data.hasActiveNetworks = activeNetworkNames.length > 0; }

anxolin · 2026-02-25T11:36:47Z

src/utils/date.ts

+ * @param endDate The end date
+ * @returns The number of months between the two dates
+ */
+export function getMonthsDifference(startDate: Date, endDate: Date): number {


Not sure if this is the implementation you want. Si hard to review

If you have:
startDate = 31/01/2026
endDate = 01/02/2026

Your algo will show 1 month. But it was just 1 day. Is this correct?

If you have:
startDate = 01/01/2026
endDate = 02/01/2026

It will return 0 months, but its just 1 day too.

anxolin · 2026-02-25T11:43:35Z

src/api/solver/content-types/solver/lifecycles.ts

+      const joinedDate = new Date(bondingPool.joinedOn);
+      const monthsDifference = getMonthsDifference(joinedDate, currentDate);
+
+      if (bondingPool.name === "CoW" && solver.isColocated === "No") {


This logic seems to fragile to me. So you are following an undocumented naming convention, that is hidden in a method of this lifecycle.

Also, this method suffers from the same things I mentioned here #85 (comment)

called calculate, but it mutates

does too many things, should rely on somethign th

has a lot of logic that for me is hard to tell if this is what you intended to do. You have bussiness logic mixed there that I can't verify (hardcoded 3 months, skip if joinedOn is missing, ans different rules for collocated cow and for others). How can I tell what I need to verify?

tamir-cow added 3 commits February 18, 2026 10:43

Added lifecycle hooks to solver table

5892d45

Updated lifecycle hooks to solver table

a7e2fb4

Updated lifecycle hooks to solver table

7ea0948

anxolin reviewed Feb 18, 2026

View reviewed changes

Addressed cr notes

b15c260

bram-vdberg reviewed Feb 25, 2026

View reviewed changes

tamir-cow requested a review from anxolin February 25, 2026 11:32

anxolin reviewed Feb 25, 2026

View reviewed changes

Comments

Conversation

tamir-cow commented Feb 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anxolin commented Feb 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anxolin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tamir-cow commented Feb 18, 2026 •

edited

Loading

anxolin commented Feb 18, 2026 •

edited

Loading