Skip to content
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

Attestations V2 #23

Open
wants to merge 138 commits into
base: master
Choose a base branch
from
Open

Attestations V2 #23

wants to merge 138 commits into from

Conversation

monkins1010
Copy link
Contributor

No description provided.

Copy link
Contributor

@michaeltout michaeltout left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR. I've been busy so unfortunately has taken a while to get to it, but the PR is overall moving in a good direction. Still requires changes, as noted in the inline comments. Let me know if anything is unclear or you have any questions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Please run through a code formatter to fix inconsistent spacing
  2. These are not actual tests, they lack the expect() function so they don't have any failure condition except an error being thrown
  3. The names for these tests do not describe what they are doing, e.g. the "request profile information" test doesn't actually request profile information, it creates a profile information request (when fixed it should probably be '(de)serialises a profile information request')
  4. Please follow the conventions of other tests and test that serialisation/deserialisation occurs successfully (i.e. a serialised object can be deserialised and remain the same, see other tests)
  5. Please remove the commented code or complete what it is a TODO for

import { SignatureData } from "../../vdxf/classes/SignatureData";

describe('Create a personal info request', () => {
test('serialize datadescriptor with nested datadescriptor', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test doesn't actually test for serialisation working as expected. Instead, it just tests for a serialised data descriptor matching some hardcoded value. Please change this to match the structure of previous serialisation tests where a data descriptor is serialised, deserialised, re-serialised, and then matched against a separate instance of the data descriptor that should contain the same data (see other tests). Feel free to reach out if this is unclear.

The goal should be to avoid hardcoding expected values where able, its mostly done in other tests only in cases where those values can only be obtained from the daemon.


});

test('serialize mmrdescriptor', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test has the same issue described above.


});

test('serialize mmrdescriptor', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test has the same issue described above.

expect(mmrbuffer).toStrictEqual("01050101002030395e0868b1953477e14bb5c16349622239a94115a4d1f02b72b6ecf8b1c79c01005741d826d3c6cbbc3a96992670d2f604e959fd1a8c014102f1eac8f180bee7fb256b801b219e20612fbc9f5e99da111a8364d1197ff3e3fbef8259770f618dacee9489e8cd2cd5dd77d36ede6c42cebdabd85a5b5e8af60b0201024c08a2ebb2c55f83a8e2a426a53320ed4d42124f4d013601600543687269732269344771736f7448476134637a436474673264384656484b664a467a56794250724d0a746578742f706c61696e204f66603f256d3f757b6dc3ea44802d4041d2a1901e06005028fd60b85a5878a201024e08a2ebb2c55f83a8e2a426a53320ed4d42124f4d01380160074d6f6e6b696e73226948796254724e42316b5852726a7343744a5864366676424b786570714d7053355a0a746578742f706c61696e2062fae0c46b2ad1177749e25fd6d48ccb40213d3cc72e4b2b0dc533039cbe8314");

});
test('serialize mmrdescriptor', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test has the same issue described above.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Please space consistently, separating exports and keeping line indents standardized with the rest of the codebase
  2. Why are these functions as opposed to constants? We can discuss this, not opposed just wondering why
  3. This filename should be all lowercase to match the rest of the codebase

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Please keep capitalisation on exports consistent
  2. Please space export as they are spaced in the rest of the codebase for readability

qualifiedname: {
namespace: "i5w5MuNik5NtLcYmNzcvaoixooEebB6MGV",
name: "vrsc::profile.data.view"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this have the identity prefix? Idk for sure, we can discuss

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To discuss maybe

vrsc::identity.profile.proposed.data.view
not sure, your basically asking if you can see your PII data that's in the mobile app

src/vdxf/keys.ts Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See note above regarding bounddata

package.json Outdated
@@ -1,6 +1,6 @@
{
"name": "verus-typescript-primitives",
"version": "1.0.0",
"version": "1.0.1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't worry about changing the version in the PR, I'll rebase the PR into a new branch and change the version when it goes to master

@monkins1010 monkins1010 force-pushed the attest branch 2 times, most recently from 1bf27c8 to dff3b51 Compare August 20, 2024 16:09
@monkins1010
Copy link
Contributor Author

When we do toJson and fromJson in classes these are not compatible with the data coming out of the daemon, how do we handle for when you e.g.
make an object to be signed, put it into the daemon, the daemon returns a json object. we want to capture that object into javascript, the json is incompatible for exmple:

 static fromJson(data: TransferDestinationJson): TransferDestination {
   return new TransferDestination({
     type: new BN(data.type),
     destination_bytes: Buffer.from(data.destination_bytes, 'hex'),
     gateway_id: data.gateway_id,
     gateway_code: data.gateway_code,
     fees: new BN(data.fees),
     aux_dests: data.aux_dests.map(x => TransferDestination.fromJson(x))
   })
 }```

@monkins1010
Copy link
Contributor Author

When we do toJson and fromJson in classes these are not compatible with the data coming out of the daemon, how do we handle for when you e.g. make an object to be signed, put it into the daemon, the daemon returns a json object. we want to capture that object into javascript, the json is incompatible for exmple:

 static fromJson(data: TransferDestinationJson): TransferDestination {
   return new TransferDestination({
     type: new BN(data.type),
     destination_bytes: Buffer.from(data.destination_bytes, 'hex'),
     gateway_id: data.gateway_id,
     gateway_code: data.gateway_code,
     fees: new BN(data.fees),
     aux_dests: data.aux_dests.map(x => TransferDestination.fromJson(x))
   })
 }```

Do we make for example a fromDaemonJson() and toDaemonJson() for each object?

@monkins1010
Copy link
Contributor Author

When we do toJson and fromJson in classes these are not compatible with the data coming out of the daemon, how do we handle for when you e.g. make an object to be signed, put it into the daemon, the daemon returns a json object. we want to capture that object into javascript, the json is incompatible for exmple:

 static fromJson(data: TransferDestinationJson): TransferDestination {
   return new TransferDestination({
     type: new BN(data.type),
     destination_bytes: Buffer.from(data.destination_bytes, 'hex'),
     gateway_id: data.gateway_id,
     gateway_code: data.gateway_code,
     fees: new BN(data.fees),
     aux_dests: data.aux_dests.map(x => TransferDestination.fromJson(x))
   })
 }```

Do we make for example a fromDaemonJson() and toDaemonJson() for each object?

I've made all the new PBaaS Classes are compatible with the daemon

Copy link
Contributor

@michaeltout michaeltout left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes! Looking good, things are coming along well, most issues concern either something we discussed on the phone, adding tests for certain components, or minor naming/style changes.

@@ -15,7 +16,8 @@ export type ApiPrimitive =
| Array<ApiPrimitive>
| IdentityDefinition
| BlockInfo
| RawTransaction;
| RawTransaction
| signDataArgs;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why this is necessary, I think everything in signDataArgs is already covered here. I deleted this like and it still builds and seems to function as expected. If this causes problems in verusd-rpc-ts-client its fine to include it here though, just capitalize the first letter to keep with convention, and make it an interface instead of a type if it can be one.

import { DataDescriptor } from "../../../utils/types/DataDescriptor";
import { SignData } from "../../../utils/types/SignData";

export type signDataArgs = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exported types typically start with a capital letter.

import { DataDescriptor } from "../../../utils/types/DataDescriptor";

export class SignDataResponse extends ApiResponse {
result: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many of these are optional as a response from signdata, best to mark those with a ?. I've been burned before because I forgot this on some other APIs (I think some still have this issue).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This component is not represented in any test file, please add tests for it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment I don't believe is addressed yet. I see that you've moved vectorEncodeVDXFUni (and the vice-versa functions) from an exported function, to a static function in the VdxfUniValue class. What i had in mind is that the vectorEncodeVDXFUni (and the related other functions) should be fully replaced by being integrated into the fromBuffer/toBuffer functions of VdxfUniValue. There should be no vectorEncodeVDXFUni, VDXFDataToUniValue, or VDXFDataToUniValueArray functions after this change, as their functionality should be covered in toBuffer/fromBuffer. I don't mind if you change the type of the "values" property to achieve this. Maybe we can call to discuss.

@@ -0,0 +1,8 @@
export type SignData = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to SignDataParameters

@@ -0,0 +1,9 @@
export type Signature = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to SignatureDataInfo

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll revisit this when the changes we discussed are made regarding handling of attestations in login consent requests/responses.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename this file to all lowercase to match its surrounding files

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename this file to all lowercase to match its surrounding files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants