-
Notifications
You must be signed in to change notification settings - Fork 33
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
[Human App] Implemented oracle filter #2697
base: develop
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
packages/apps/human-app/server/src/modules/oracle-discovery/oracle-discovery.service.ts
Outdated
Show resolved
Hide resolved
|
||
private filterOracles(foundOracles: IOperator[], jobTypes: string[]) { | ||
if (foundOracles && foundOracles.length > 0) { | ||
const filteredOracles = foundOracles.filter((oracle) => { |
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.
why are you filtering the oracles if they dont have url?
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.
Check is here:
const filteredOracles = oracles.filter((oracle) => {
if (!oracle.url || oracle.url === null) {
return false;
}
return true;
});
If oracle don't have .url
we skip it.
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.
Any suggestions here?
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.
I just dont see a reason to filter out the ones that don't have a url
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.
I will try to find more efficient way
packages/apps/human-app/server/src/modules/oracle-discovery/oracle-discovery.service.ts
Outdated
Show resolved
Hide resolved
|
||
if (!jobTypes || jobTypes === '') { | ||
throw new HttpException( | ||
`Unable to retrieve job types from address: ${address}`, |
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.
Lets not throw an exception if the job types are not set, just return an empty array or undefined
) {} | ||
|
||
async processOracleDiscovery( | ||
command: OracleDiscoveryCommand, | ||
): Promise<OracleDiscoveryResponse[]> { | ||
const address = this.configService.reputationOracleAddress; | ||
const chainIds = this.configService.chainIdsEnabled; | ||
const jobTypesByChainId = new Map<ChainId, string[]>(); |
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.
this variable is not used
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.
It used here:
jobTypesByChainId.set(+chainId, jobTypes);
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.
yes, you are setting the values but never using it
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.
I see what do you mean, I used it before optimised previous solution, thanks
@@ -49,4 +49,42 @@ export class KvStoreGateway { | |||
return fetchedData; | |||
} | |||
} | |||
|
|||
async getJobTypesByAddress(address: string): Promise<string | void> { |
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.
we should use the chainId
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.
chainId
used inside getJobTypesByAddress
method.
const chainId: ChainId = Number(network?.chainId);
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.
I mean pass it as parameter
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.
Do we really need this? Since we have only one chainId
assigned to rpcUrl
in configs.
Description
Enhanced the Oracle Discovery Service by improving the filtering mechanism for oracles based on their supported job types. The goal is to ensure that only relevant oracles are returned, optimizing the discovery process.
Summary of changes
processOracleDiscovery
andfindOraclesByChainId
methods to OracleDiscoveryService.filterOracles
andmatchesJobTypes
methods to OracleDiscoveryService.How test the changes
yarn test
Related issues
[HUMAN App] Filter Exchange Oracles by job types allowed in the reputation network
#2613