Critical code review PR - Last sprint handover checklist of 0.2.0 release

[vishwa-vyom]

This PR is created just to add the review comments as part of the critical code review task of last sprint handover checklist for release of 0.2.0 version.

** THIS PR SHOULD NOT BE MERGED **

[vishwa-vyom]

Can we make this section little more eloborate to call out this is a library, it be used used by android / java application to get the functionality of OpenID4VCI interoperablity etc.. ?

[swatigoel]

Sure @vishwa-vyom will update it

[vishwa-vyom]

When DownloadFailedException is thrown do we also have a list of error codes that a consumer app receive to take respective actions ?

[KiruthikaJeyashankar]

Download failed exception is thrown in

• Certify credential endpoint failure (error response is added as message)
• Common fallback
  The message field of the exception holds the error message from the issuer but we do not have any predefined / extracted error codes being passed like https://github.com/mosip/vc-verifier/tree/develop/vc-verifier/kotlin#:~:text=**%20Mdoc%20Format%20VC,ERR_GENERIC

[vishwa-vyom]

should we document the same in this readme file ?

[vishwa-vyom]

I think CREDENTIAL_ENDPOINT and CREDENTIAL_AUDIENCE will be mostly same, we can verify this understanding and remove the audience if this is true to reduce the integration complexity.

[vishwa-vyom]

DOWNLOAD_TIMEOUT does not natually fit into the IssuerMetaData class umbraella. We can keep this as another argument in the method.

[swatigoel]

yes, we will handle it as part of release-0.17.0 of Inji mobile.

[vishwa-vyom]

with and without proof, can we keep this as overrided method ?
also the proof generation string that is to be generated before signing can be exposed a helper method for cases where the signing handle cannot be passed inside.

[vishwa-vyom]

Typical factory design pattern implementation will give out a instance, but here we can see the method invocation also happens. If there is no specific reason, we should restructure the code according to proper factory design pattern.

[swatigoel]

agree @vishwa-vyom
@KiruthikaJeyashankar let's try it out, something like this

abstract class Factory {
	abstract Factory getInstance(String format);
	abstract void validate();
	abstract void buildRequest();
}


class LdpVC extends Factory {
	
	Factory getInstance(String format) {
		if (format.equals("ldp_vc")) {
			return new LdpVC();
		}
	}

	public void validate() {
	
	}
}



class MsoMdoc extends Factory {
	
	Factory getInstance(String format) {
		if (format.equals("mso_mdoc")) {
			return new MsoMdoc();
		}
	}

	public void validate() {

	}
}


class VCIClient {
	
	public void init(String format) {
		Factory factory = Factory.getInstance(format);

		factory.validate();
	}
}

[vishwa-vyom]

This factory class implemention another factory class is confusing. Any specific reason ?

[swatigoel]

We will refactor the code the way mentioned here #43 (comment)