Added required changes for log messages #18
Conversation
|
@Girishbari Added few comments for the PR. Please resolve them. Also please post a demo video showcasing different log types when each of the log level is set while initialising the object. It will serve as a proof of functionality and help us to merge the PR readily. |
|
where have you added comments in PR @Kushal7788 |
|
In prev PR there were some comments so I made changes accordingly and pushed new changes |
src/Reclaim.ts
Outdated
| @@ -59,7 +59,7 @@ export async function verifyProof(proof: Proof): Promise<boolean> { | |||
| proof.claimData.timestampS | |||
| ) | |||
| } | |||
| // then hash the claim info with the encoded ctx to get the identifier | |||
| // then hash the claim error with the encoded ctx to get the identifier | |||
There was a problem hiding this comment.
Don't change the comments details. This is not related to logging and serves a different purpose
src/Reclaim.ts
Outdated
| return false | ||
| } | ||
|
|
||
| return true | ||
| } | ||
|
|
||
| export function transformForOnchain(proof: Proof): { claimInfo: any, signedClaim: any } { | ||
| const claimInfoBuilder = new Map([ | ||
| export function transformForOnchain(proof: Proof): { claimerror: any, signedClaim: any } { |
There was a problem hiding this comment.
The parameter name should not be updated here. Please make sure you are not find & replacing all Info keywords with error keywords
src/Reclaim.ts
Outdated
| export function transformForOnchain(proof: Proof): { claimInfo: any, signedClaim: any } { | ||
| const claimInfoBuilder = new Map([ | ||
| export function transformForOnchain(proof: Proof): { claimerror: any, signedClaim: any } { | ||
| const claimerrorBuilder = new Map([ |
There was a problem hiding this comment.
This should not be changes
src/Reclaim.ts
Outdated
| ['context', proof.claimData.context], | ||
| ['parameters', proof.claimData.parameters], | ||
| ['provider', proof.claimData.provider], | ||
| ]); | ||
| const claimInfo = Object.fromEntries(claimInfoBuilder); | ||
| const claimerror = Object.fromEntries(claimerrorBuilder); |
There was a problem hiding this comment.
Unnecessary change. Please revert
src/Reclaim.ts
Outdated
| @@ -108,7 +108,7 @@ export function transformForOnchain(proof: Proof): { claimInfo: any, signedClaim | |||
| claim: Object.fromEntries(claimBuilder), | |||
| signatures: proof.signatures, | |||
| }; | |||
| return { claimInfo, signedClaim }; | |||
| return { claimerror, signedClaim }; | |||
There was a problem hiding this comment.
Unnecessary change. Please revert
src/Reclaim.ts
Outdated
| @@ -296,7 +304,8 @@ export class ReclaimProofRequest { | |||
| validateFunctionParams([{ input: this.sessionId, paramName: 'sessionId', isString: true }], 'getAppCallbackUrl'); | |||
| return this.appCallbackUrl || `${constants.DEFAULT_RECLAIM_CALLBACK_URL}${this.sessionId}` | |||
| } catch (error) { | |||
| logger.info("Error getting app callback url", error) | |||
| logger.error("Error getting app callback url", error) | |||
| logger.info(`Make sure to pass URL and as a string`) | |||
There was a problem hiding this comment.
This info log should be not be in the catch block. Better to move it at top inside the try block
src/Reclaim.ts
Outdated
| @@ -306,7 +315,8 @@ export class ReclaimProofRequest { | |||
| validateFunctionParams([{ input: this.sessionId, paramName: 'sessionId', isString: true }], 'getStatusUrl'); | |||
| return `${constants.DEFAULT_RECLAIM_STATUS_URL}${this.sessionId}` | |||
| } catch (error) { | |||
| logger.info("Error fetching Status Url", error) | |||
| logger.error("Error fetching Status Url", error) | |||
| logger.info(`Make sure to pass URL and as a string`) | |||
There was a problem hiding this comment.
This info log should be not be in the catch block. Better to move it at top inside the try block
| this.level = level; | ||
| } | ||
|
|
||
| private shouldLog(messageLevel: LogLevel): boolean { | ||
| const levels: LogLevel[] = ['error', 'warn', 'info', 'silent']; | ||
| const levels: ExtendedLog[] = ['error', 'warn', 'info', 'all']; |
There was a problem hiding this comment.
Will this log all there types of log 'error', 'warn', 'info' when logLevel is set to info ?
There was a problem hiding this comment.
I didnt get your question
src/utils/validationUtils.ts
Outdated
| import { ProofRequestOptions } from "./types"; | ||
| const logger = loggerModule.logger; | ||
|
|
||
| const REFERENCE_FOR_DASHBOARD = "https://dev.reclaimprotocol.org/dashboard" |
There was a problem hiding this comment.
Why is this needed here?
There was a problem hiding this comment.
Not needed will remove it
src/utils/validationUtils.ts
Outdated
| @@ -39,7 +44,8 @@ export function validateURL(url: string, functionName: string): void { | |||
| try { | |||
| new URL(url); | |||
| } catch (e) { | |||
| logger.info(`URL validation failed for ${url} in ${functionName}: ${(e as Error).message}`); | |||
| logger.error(`URL validation failed for ${url} in ${functionName}: ${(e as Error).message}`); | |||
| logger.info('Make sure to pass URL as string') | |||
There was a problem hiding this comment.
This info log should be not be in the catch block. Better to move it at top inside the try block
Description
I have added required changes as per my knowledge and understanding for below ISSUE
PR for #6
Type of change
Checklist: