-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature cluster matching #1862
Feature cluster matching #1862
Changes from 8 commits
b20f60d
d0b4b97
56857c3
94a77c7
e42dbb1
1a65357
fc69928
5496642
bfa1d12
f0a04db
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
import { MigrationInterface, QueryRunner } from 'typeorm'; | ||
|
||
export class AddEstimatedClusterMatching1728554628004 | ||
implements MigrationInterface | ||
{ | ||
public async up(queryRunner: QueryRunner): Promise<void> { | ||
await queryRunner.query(` | ||
CREATE TABLE estimated_cluster_matching ( | ||
id SERIAL PRIMARY KEY, | ||
project_id INT NOT NULL, | ||
qf_round_id INT NOT NULL, | ||
matching DOUBLE PRECISION NOT NULL | ||
); | ||
`); | ||
|
||
// Create indexes on the new table | ||
await queryRunner.query(` | ||
CREATE INDEX estimated_cluster_matching_project_id_qfround_id | ||
ON estimated_cluster_matching (project_id, qf_round_id); | ||
`); | ||
|
||
await queryRunner.query(` | ||
CREATE INDEX estimated_cluster_matching_matching | ||
ON estimated_cluster_matching (matching); | ||
`); | ||
} | ||
|
||
public async down(queryRunner: QueryRunner): Promise<void> { | ||
// Revert changes if necessary by dropping the table and restoring the view | ||
await queryRunner.query(` | ||
DROP TABLE IF EXISTS estimated_cluster_matching; | ||
`); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,46 @@ | ||||||||||||||||||||||||||||||||||||||||||||
import axios from 'axios'; | ||||||||||||||||||||||||||||||||||||||||||||
import { | ||||||||||||||||||||||||||||||||||||||||||||
CocmAdapterInterface, | ||||||||||||||||||||||||||||||||||||||||||||
EstimatedMatchingInput, | ||||||||||||||||||||||||||||||||||||||||||||
ProjectsEstimatedMatchings, | ||||||||||||||||||||||||||||||||||||||||||||
} from './cocmAdapterInterface'; | ||||||||||||||||||||||||||||||||||||||||||||
import { logger } from '../../utils/logger'; | ||||||||||||||||||||||||||||||||||||||||||||
import { i18n, translationErrorMessagesKeys } from '../../utils/errorMessages'; | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
export class CocmAdapter implements CocmAdapterInterface { | ||||||||||||||||||||||||||||||||||||||||||||
private ClusterMatchingURL; | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
constructor() { | ||||||||||||||||||||||||||||||||||||||||||||
this.ClusterMatchingURL = | ||||||||||||||||||||||||||||||||||||||||||||
process.env.CLUSTER_MATCHING_API_URL || 'localhost'; | ||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+14
to
+15
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security: Improve default configuration for production use The default URL of 'localhost' could be problematic in production. Consider:
- this.ClusterMatchingURL =
- process.env.CLUSTER_MATCHING_API_URL || 'localhost';
+ if (!process.env.CLUSTER_MATCHING_API_URL) {
+ throw new Error('CLUSTER_MATCHING_API_URL environment variable is required');
+ }
+ this.ClusterMatchingURL = process.env.CLUSTER_MATCHING_API_URL; 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
async fetchEstimatedClusterMatchings( | ||||||||||||||||||||||||||||||||||||||||||||
matchingDataInput: EstimatedMatchingInput, | ||||||||||||||||||||||||||||||||||||||||||||
): Promise<ProjectsEstimatedMatchings> { | ||||||||||||||||||||||||||||||||||||||||||||
try { | ||||||||||||||||||||||||||||||||||||||||||||
const result = await axios.post( | ||||||||||||||||||||||||||||||||||||||||||||
this.ClusterMatchingURL, | ||||||||||||||||||||||||||||||||||||||||||||
matchingDataInput, | ||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||
headers: { | ||||||||||||||||||||||||||||||||||||||||||||
Accept: 'application/json', | ||||||||||||||||||||||||||||||||||||||||||||
'Content-Type': 'application/json', | ||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+22
to
+31
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add timeout and retry mechanism for API calls The axios request lacks timeout configuration and retry mechanism, which could lead to hanging requests or unnecessary failures. + const axiosConfig = {
+ timeout: 5000, // 5 seconds
+ headers: {
+ Accept: 'application/json',
+ 'Content-Type': 'application/json',
+ },
+ retry: 3,
+ retryDelay: (retryCount) => retryCount * 1000, // exponential backoff
+ };
const result = await axios.post(
this.ClusterMatchingURL,
matchingDataInput,
- {
- headers: {
- Accept: 'application/json',
- 'Content-Type': 'application/json',
- },
- },
+ axiosConfig,
);
|
||||||||||||||||||||||||||||||||||||||||||||
if (result?.data?.error !== null) { | ||||||||||||||||||||||||||||||||||||||||||||
logger.error('clusterMatchingApi error', result.data.error); | ||||||||||||||||||||||||||||||||||||||||||||
throw new Error( | ||||||||||||||||||||||||||||||||||||||||||||
i18n.__(translationErrorMessagesKeys.CLUSTER_MATCHING_API_ERROR), | ||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
return result.data; | ||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+32
to
+38
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Improve error handling logic The error check - if (result?.data?.error !== null) {
+ if (!result?.data || result.data.error) {
logger.error('clusterMatchingApi error', result.data.error);
throw new Error(
i18n.__(translationErrorMessagesKeys.CLUSTER_MATCHING_API_ERROR),
);
}
+ // Validate response structure matches ProjectsEstimatedMatchings
+ if (!Array.isArray(result.data?.matching_data)) {
+ logger.error('Invalid response structure', result.data);
+ throw new Error(
+ i18n.__(translationErrorMessagesKeys.CLUSTER_MATCHING_API_ERROR),
+ );
+ } 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||
} catch (e) { | ||||||||||||||||||||||||||||||||||||||||||||
logger.error('clusterMatchingApi error', e); | ||||||||||||||||||||||||||||||||||||||||||||
throw new Error( | ||||||||||||||||||||||||||||||||||||||||||||
i18n.__(translationErrorMessagesKeys.CLUSTER_MATCHING_API_ERROR), | ||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
// Example Data | ||
// { | ||
// "matching_data": [ | ||
// { | ||
// "matching_amount": 83.25, | ||
// "matching_percent": 50.0, | ||
// "project_name": "Test1", | ||
// "strategy": "COCM" | ||
// }, | ||
// { | ||
// "matching_amount": 83.25, | ||
// "matching_percent": 50.0, | ||
// "project_name": "Test3", | ||
// "strategy": "COCM" | ||
// } | ||
// ] | ||
// } | ||
|
||
export interface ProjectsEstimatedMatchings { | ||
matching_data: { | ||
matching_amount: number; | ||
matching_percent: number; | ||
project_name: string; | ||
strategy: string; | ||
}[]; | ||
} | ||
|
||
export interface EstimatedMatchingInput { | ||
votes_data: [ | ||
{ | ||
voter: string; | ||
payoutAddress: string; | ||
amountUSD: number; | ||
project_name: string; | ||
score: number; | ||
}, | ||
]; | ||
strategy: string; | ||
min_donation_threshold_amount: number; | ||
matching_cap_amount: number; | ||
matching_amount: number; | ||
passport_threshold: number; | ||
} | ||
|
||
export interface CocmAdapterInterface { | ||
fetchEstimatedClusterMatchings( | ||
matchingDataInput: EstimatedMatchingInput, | ||
): Promise<ProjectsEstimatedMatchings>; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
import { | ||
CocmAdapterInterface, | ||
ProjectsEstimatedMatchings, | ||
} from './cocmAdapterInterface'; | ||
|
||
export class CocmMockAdapter implements CocmAdapterInterface { | ||
async fetchEstimatedClusterMatchings( | ||
_matchingDataInput, | ||
): Promise<ProjectsEstimatedMatchings> { | ||
return { | ||
matching_data: [ | ||
{ | ||
matching_amount: 83.25, | ||
matching_percent: 50.0, | ||
project_name: 'Test1', | ||
strategy: 'COCM', | ||
}, | ||
{ | ||
matching_amount: 83.25, | ||
matching_percent: 50.0, | ||
project_name: 'Test3', | ||
strategy: 'COCM', | ||
}, | ||
], | ||
}; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
import { Field, ObjectType } from 'type-graphql'; | ||
import { | ||
Column, | ||
Index, | ||
PrimaryGeneratedColumn, | ||
BaseEntity, | ||
Entity, | ||
ManyToOne, | ||
JoinColumn, | ||
} from 'typeorm'; | ||
import { Project } from './project'; | ||
|
||
@Entity('estimated_cluster_matching') | ||
@Index('estimated_cluster_matching_project_id_qfround_id', [ | ||
'projectId', | ||
'qfRoundId', | ||
]) | ||
@Index('estimated_cluster_matching_matching', ['matching']) | ||
@ObjectType() | ||
export class EstimatedClusterMatching extends BaseEntity { | ||
@Field() | ||
@PrimaryGeneratedColumn() | ||
id: number; // New primary key | ||
|
||
@Field(_type => Project) | ||
@ManyToOne(_type => Project, project => project.projectEstimatedMatchingView) | ||
@JoinColumn({ referencedColumnName: 'id' }) | ||
project: Project; | ||
|
||
@Field() | ||
@Column() | ||
projectId: number; | ||
|
||
@Field() | ||
@Column() | ||
qfRoundId: number; | ||
|
||
@Field() | ||
@Column('double precision') | ||
matching: number; | ||
Comment on lines
+38
to
+40
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider Decimal Precision for Monetary Fields
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add foreign key constraints and audit fields
The table definition is missing important constraints and audit fields.