-
Notifications
You must be signed in to change notification settings - Fork 27
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
Remove/pkg cli deps #815
Remove/pkg cli deps #815
Conversation
Signed-off-by: Prad Nukala <prad@sonr.io>
Apply Sweep Rules to your PR?
|
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
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.
Feedback from Senior Dev Bot
"details": { | ||
"type": "array", | ||
"items": { | ||
"$ref": "#/definitions/google.protobuf.Any" | ||
"type": "object", | ||
"$ref": "#/definitions/protobufAny" | ||
} | ||
} | ||
} |
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.
Overall, the code changes seem reasonable. However, there are a few suggestions for improvement:
-
Instead of using the generic "type" property for the object, consider being more specific and providing a descriptive name for the type. This can help improve code readability and understanding. For example, "protobufAny" can be replaced with a more meaningful name like "detailsObject".
-
Consider updating the references to the new object definition consistently across the codebase, so that it is easier to understand and maintain.
"type": "string" | ||
} | ||
], | ||
"tags": [ | ||
"AuthenticationService" | ||
] | ||
"tags": ["AuthenticationService"] | ||
} | ||
}, | ||
"/highway/auth/jwt/verify/{jwt}": { |
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.
In the Git diff, I see that you have made changes to the tags property. It looks like you have removed the unnecessary line breaks and adjusted the quotes for the array elements. However, I would recommend keeping a consistent coding style throughout the file.
Here's an improved version:
"tags": ["AuthenticationService"],
By removing the unnecessary line breaks within the array and keeping the quotes consistent, the code becomes more readable.
"details": { | ||
"type": "array", | ||
"items": { | ||
"$ref": "#/definitions/google.protobuf.Any" | ||
"type": "object", | ||
"$ref": "#/definitions/protobufAny" | ||
} | ||
} | ||
} |
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.
The code changes look good overall. However, it would be better to use a more descriptive name for the $ref
value in the items
field. Also, it is recommended to use the properties
field instead of items
if you're defining an object type. Here's an improved version of the code:
{
"details": {
"type": "array",
"items": {
"type": "object",
"properties": {
"protobufAny": {
"$ref": "#/definitions/protobufAny"
}
}
}
}
}
"details": { | ||
"type": "array", | ||
"items": { | ||
"$ref": "#/definitions/google.protobuf.Any" | ||
"type": "object", | ||
"$ref": "#/definitions/protobufAny" | ||
} | ||
} | ||
} |
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.
The code changes look good overall. However, I would suggest a couple of improvements:
- Instead of using the deprecated "google.protobuf.Any" type, it's better to use the new "protobufAny" type.
"type": "object",
"$ref": "#/definitions/protobufAny"
- Consider adding a description for the "type" property to provide clarity on what it represents.
"description": "Details of the item",
"type": "object",
"$ref": "#/definitions/protobufAny"
These changes will make the code more maintainable and easier to understand for future developers.
"format": "uint64" | ||
}, | ||
{ | ||
"name": "pagination.count_total", | ||
"name": "pagination.countTotal", | ||
"description": "count_total is set to true to indicate that the result set should include\na count of the total number of items available for pagination in UIs.\ncount_total is only respected when offset is used. It is ignored when key\nis set.", | ||
"in": "query", | ||
"required": false, |
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.
The change from "pagination.count_total" to "pagination.countTotal" is a good improvement because it follows the camel case naming convention which is commonly used in code. It's important to maintain consistent naming conventions to improve code readability and maintainability.
{
"name": "pagination.countTotal",
"description": "count_total is set to true to indicate that the result set should include\na count of the total number of items available for pagination in UIs.\ncount_total is only respected when offset is used. It is ignored when key\nis set.",
"in": "query",
"required": false,
"type": "boolean"
}
"200": { | ||
"description": "A successful response.", | ||
"schema": { | ||
"$ref": "#/definitions/highway.authentication.v1.LoginResponse" | ||
"$ref": "#/definitions/v1LoginResponse" | ||
} | ||
}, | ||
"default": { | ||
"description": "An unexpected error response.", | ||
"schema": { | ||
"$ref": "#/definitions/grpc.gateway.runtime.Error" | ||
"$ref": "#/definitions/rpcStatus" | ||
} | ||
} | ||
}, | ||
"parameters": [ | ||
{ | ||
"name": "origin", | ||
"description": "The origin of the request.", | ||
"in": "path", | ||
"required": true, | ||
"type": "string" | ||
}, | ||
{ | ||
"name": "alias", | ||
"description": "The user's alias.", | ||
"in": "query", | ||
"required": false, | ||
"type": "string" | ||
}, | ||
{ | ||
"name": "assertion", | ||
"description": "The user's assertion.", | ||
"in": "query", | ||
"required": false, | ||
"type": "string" | ||
} | ||
], | ||
"tags": [ | ||
"AuthenticationService" | ||
] | ||
"tags": ["AuthenticationService"] | ||
} | ||
}, | ||
"/highway/auth/params/{origin}": { |
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.
- In the first code change, the
$ref
value has been updated from#definitions/highway.authentication.v1.LoginResponse
to#definitions/v1LoginResponse
. It would be better to use more descriptive and meaningful names for the reference, such as#definitions/LoginResponse
or#definitions/AuthenticationResponse
.
"schema": {
"$ref": "#/definitions/LoginResponse"
}
- In the second code change, the
$ref
value has been updated from#definitions/grpc.gateway.runtime.Error
to#definitions/rpcStatus
. Again, it would be better to use more descriptive names, such as#definitions/ErrorResponse
or#definitions/ErrorStatus
.
"schema": {
"$ref": "#/definitions/ErrorResponse"
}
- In the parameters section, new parameters have been added with descriptions. It is good practice to provide clear and informative descriptions for each parameter. However, the existing parameters lack descriptions. It would be helpful to add descriptions for them as well.
"parameters": [
{
"name": "origin",
"description": "The origin of the request.",
"in": "path",
"required": true,
"type": "string"
},
{
"name": "alias",
"description": "The user's alias.",
"in": "query",
"required": false,
"type": "string"
},
{
"name": "assertion",
"description": "The user's assertion.",
"in": "query",
"required": false,
"type": "string"
}
]
- In the
tags
section, the array has been updated to["AuthenticationService"]
. It is good to keep tags in an array format for future extensibility. However, the tags should be aligned properly for better readability.
"tags": [
"AuthenticationService"
]
Overall, the code changes look fine, but they can be improved by using more descriptive and meaningful names and providing clear parameter descriptions.
"200": { | ||
"description": "A successful response.", | ||
"schema": { | ||
"$ref": "#/definitions/highway.authentication.v1.RegisterResponse" | ||
"$ref": "#/definitions/v1RegisterResponse" | ||
} | ||
}, | ||
"default": { | ||
"description": "An unexpected error response.", | ||
"schema": { | ||
"$ref": "#/definitions/grpc.gateway.runtime.Error" | ||
"$ref": "#/definitions/rpcStatus" | ||
} | ||
} | ||
}, | ||
"parameters": [ | ||
{ | ||
"name": "origin", | ||
"description": "The origin of the request.", | ||
"in": "path", | ||
"required": true, | ||
"type": "string" | ||
}, | ||
{ | ||
"name": "username", | ||
"description": "The user's username.", | ||
"in": "query", | ||
"required": false, | ||
"type": "string" | ||
}, | ||
{ | ||
"name": "attestation", | ||
"description": "The user's attestation.", | ||
"in": "query", | ||
"required": false, | ||
"type": "string" | ||
}, | ||
{ | ||
"name": "challenge", | ||
"description": "The challenge for the user.", | ||
"in": "query", | ||
"required": false, | ||
"type": "string" | ||
} | ||
], | ||
"tags": [ | ||
"AuthenticationService" | ||
] | ||
"tags": ["AuthenticationService"] | ||
} | ||
} | ||
}, | ||
"definitions": { | ||
"google.protobuf.Any": { | ||
"protobufAny": { | ||
"type": "object", | ||
"properties": { | ||
"type_url": { | ||
"@type": { | ||
"type": "string" | ||
}, | ||
"value": { | ||
"type": "string", | ||
"format": "byte" | ||
} | ||
} | ||
}, | ||
"additionalProperties": {} | ||
}, | ||
"grpc.gateway.runtime.Error": { | ||
"rpcStatus": { | ||
"type": "object", | ||
"properties": { | ||
"error": { | ||
"type": "string" | ||
}, | ||
"code": { | ||
"type": "integer", | ||
"format": "int32" |
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.
Based on the provided Git diff, here are some suggestions to improve the code changes:
-
Schema definitions should have clear and concise names that reflect their purpose. Update the
$ref
value to reflect the version and purpose of the schema. For example:- "$ref": "#/definitions/highway.authentication.v1.RegisterResponse" + "$ref": "#/definitions/v1RegisterResponse"
becomes:
- "$ref": "#/definitions/highway.authentication.v1.RegisterResponse" + "$ref": "#/definitions/v1.RegisterResponse"
-
When introducing new parameters, ensure that meaningful descriptions are provided to improve code readability and maintainability. For example:
+ { + "name": "origin", + "description": "The origin of the request.", + "in": "path", + "required": true, + "type": "string" + }
-
Use consistent naming conventions for parameters, such as camelCase. For example:
+ { + "name": "username", + "description": "The user's username.", + "in": "query", + "required": false, + "type": "string" + }
-
Avoid unnecessary nesting in schema definitions. For example:
- "google.protobuf.Any": { + "protobufAny": {
-
Remove unnecessary properties and formats from schema definitions. For example:
- "value": { - "type": "string", - "format": "byte" - }
becomes:
"additionalProperties": {}
-
Keep the code formatting consistent. For example, instead of:
"tags": [ "AuthenticationService" ]
use:
"tags": ["AuthenticationService"]
With these changes, the code will be easier to understand and maintain in the long run.
} | ||
}, | ||
"description": "PageResponse is to be embedded in gRPC response messages where the\ncorresponding request message has used PageRequest.\n\n message SomeResponse {\n repeated Bar results = 1;\n PageResponse page = 2;\n }" | ||
}, | ||
"google.protobuf.Any": { | ||
"type": "object", | ||
"properties": { | ||
"type_url": { | ||
"type": "string" | ||
}, | ||
"value": { | ||
"type": "string", | ||
"format": "byte" | ||
} | ||
} | ||
}, | ||
"grpc.gateway.runtime.Error": { | ||
"type": "object", | ||
"properties": { | ||
"error": { | ||
"type": "string" | ||
}, | ||
"code": { | ||
"type": "integer", | ||
"format": "int32" | ||
}, | ||
"message": { | ||
"type": "string" | ||
}, | ||
"details": { | ||
"type": "array", | ||
"items": { | ||
"$ref": "#/definitions/google.protobuf.Any" | ||
} | ||
} | ||
} | ||
} | ||
} | ||
} |
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.
Based on the provided diff, it seems that some parts of the code have been removed. Specifically, the definitions for "google.protobuf.Any"
and "grpc.gateway.runtime.Error"
have been deleted. It would be helpful to understand the reasoning behind this removal in order to provide more specific feedback. However, in general, it is important to ensure that any code changes are properly reviewed and tested to avoid any unintended consequences.
Hint:
Make sure you have thoroughly reviewed the impact of removing these definitions and consider if they are truly unnecessary. If they are indeed unnecessary, you can remove the entire section.
Here is an example of the updated code after removing the section:
{
"openapi": "3.0.0",
"info": {
"title": "Your API",
"version": "1.0.0"
},
"paths": {
"/somepath": {
"get": {
"operationId": "getSomePath",
"responses": {
"200": {
"description": "OK",
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/SomeResponse"
}
}
}
}
}
}
}
},
"components": {
"schemas": {
"SomeResponse": {
"type": "object",
"properties": {
"results": {
"type": "array",
"items": {
"$ref": "#/components/schemas/Bar"
}
},
"page": {
"$ref": "#/components/schemas/PageResponse"
}
}
},
"Bar": {
"type": "object",
"properties": {
"name": {
"type": "string"
}
}
},
"PageResponse": {
"type": "object",
"properties": {
"pageNumber": {
"type": "integer",
"format": "int32"
},
"pageSize": {
"type": "integer",
"format": "int32"
},
"totalElements": {
"type": "integer",
"format": "int64"
}
}
}
}
}
}
}, | ||
"type": { | ||
"type": "string" | ||
"type": "string", | ||
"title": "The type of the verification method" | ||
}, | ||
"controller": { | ||
"type": "string" | ||
"type": "string", | ||
"title": "The entity controlling the verification method" | ||
}, | ||
"public_key_jwk": { | ||
"type": "string" | ||
"publicKeyJwk": { | ||
"type": "string", | ||
"title": "optional, the public key in JSON Web Key (JWK) format" | ||
}, | ||
"public_key_multibase": { | ||
"type": "string" | ||
"publicKeyMultibase": { | ||
"type": "string", | ||
"title": "optional, the public key in multibase-encoded format" | ||
}, | ||
"blockchain_account_id": { | ||
"type": "string" | ||
"blockchainAccountId": { | ||
"type": "string", | ||
"title": "optional, the blockchain account identifier associated with the" | ||
}, | ||
"attestation_type": { | ||
"type": "string" | ||
"attestationType": { | ||
"type": "string", | ||
"title": "optional, the type of attestation associated with the verification" | ||
}, | ||
"transports": { | ||
"type": "array", | ||
"items": { | ||
"type": "string" | ||
} | ||
}, | ||
"title": "optional, the set of transport protocols supported by the" | ||
} | ||
}, | ||
"description": "VerificationMethod represents a verification method that can be used to\nauthenticate the DID subject or perform other cryptographic operations." | ||
}, | ||
"core.identity.VerificationRelationship": { | ||
"identityVerificationRelationship": { | ||
"type": "object", | ||
"properties": { | ||
"verification_method": { | ||
"$ref": "#/definitions/core.identity.VerificationMethod" | ||
"verificationMethod": { | ||
"$ref": "#/definitions/identityVerificationMethod", | ||
"title": "The verification method associated with the relationship" | ||
}, | ||
"reference": { | ||
"type": "string" | ||
"type": "string", | ||
"title": "The reference identifier for the relationship" | ||
}, | ||
"type": { | ||
"type": "string" | ||
"type": "string", | ||
"title": "The type of the verification relationship" | ||
}, | ||
"owner": { | ||
"type": "string" | ||
"type": "string", | ||
"title": "The idx address of the owner of the verification relationship" | ||
} | ||
}, | ||
"description": "VerificationRelationship represents a relationship between a verification\nmethod and a specific verification purpose (e.g., authentication, assertion,\netc.)." | ||
}, | ||
"google.protobuf.Any": { | ||
"protobufAny": { | ||
"type": "object", | ||
"properties": { | ||
"type_url": { | ||
"@type": { | ||
"type": "string" | ||
}, | ||
"value": { | ||
"type": "string", | ||
"format": "byte" | ||
} | ||
} | ||
}, | ||
"additionalProperties": {} | ||
}, | ||
"grpc.gateway.runtime.Error": { | ||
"rpcStatus": { | ||
"type": "object", | ||
"properties": { | ||
"error": { | ||
"type": "string" | ||
}, | ||
"code": { | ||
"type": "integer", | ||
"format": "int32" |
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.
The code changes are generally good. However, here are a few suggestions for improvement:
-
Use camel case for property names: In the updated code, some property names are using snake case (
did_document
,also_known_as
). It is recommended to use camel case instead (didDocument
,alsoKnownAs
). -
Provide titles for properties: It would be helpful to provide titles for the properties to improve readability and make the purpose of each property clearer.
Here are the updated code changes:
"identityDIDDocument": {
"type": "object",
"properties": {
"context": {
"type": "array",
"items": {
"type": "string"
},
+ "title": "The JSON-LD context(s) used in the document"
},
"id": {
"type": "string",
+ "title": "The identifier for the DID subject"
},
"controller": {
"type": "array",
"items": {
"type": "string"
},
+ "title": "Optional, the entity/entities controlling the DID subject"
},
"authentication": {
"type": "array",
"items": {
"type": "object",
"$ref": "#/definitions/identityVerificationRelationship"
},
+ "title": "Optional, the set of authentication methods associated with the DID"
},
"assertionMethod": {
"type": "array",
"items": {
"type": "object",
"$ref": "#/definitions/identityVerificationRelationship"
},
+ "title": "Optional, the set of assertion methods associated with the DID"
},
"capabilityInvocation": {
"type": "array",
"items": {
"type": "object",
"$ref": "#/definitions/identityVerificationRelationship"
},
+ "title": "Optional, the set of capability invocation methods associated with the DID"
},
"capabilityDelegation": {
"type": "array",
"items": {
"type": "object",
"$ref": "#/definitions/identityVerificationRelationship"
},
+ "title": "Optional, the set of capability delegation methods associated with the DID"
},
"keyAgreement": {
"type": "array",
"items": {
"type": "object",
"$ref": "#/definitions/identityVerificationRelationship"
},
+ "title": "Optional, the set of key agreement methods associated with the DID"
},
"alsoKnownAs": {
"type": "array",
"items": {
"type": "string"
},
+ "title": "Optional, the set of identifiers for other personas or identities"
}
},
"description": "DIDDocument represents a Decentralized Identifier (DID) document that\ncontains information about the DID subject, such as public keys, verification\nmethods, and services."
},
"identityMsgCreateControllerAccountResponse": {
"type": "object",
"properties": {
"id": {
"type": "string",
"format": "uint64",
+ "description": "The ID of the created controller account."
},
"account": {
"$ref": "#/definitions/identityControllerAccount",
+ "description": "The created controller account."
}
},
"description": "MsgCreateControllerAccountResponse represents the response to a create\ncontroller account request."
},
"identityMsgDeleteControllerAccountResponse": {
"type": "object",
+ "description": "MsgDeleteControllerAccountResponse represents the response to a delete\ncontroller account request."
},
"identityMsgRegisterIdentityResponse": {
"type": "object",
"properties": {
"success": {
"type": "boolean",
+ "description": "Indicates if the registration was successful."
},
"didDocument": {
"$ref": "#/definitions/identityDIDDocument",
+ "description": "The DID document associated with the registered identity."
}
},
"description": "MsgRegisterIdentityResponse represents the response to a register identity\nrequest."
},
"identityMsgUpdateControllerAccountResponse": {
"type": "object",
"properties": {
"account": {
"$ref": "#/definitions/identityControllerAccount",
+ "description": "The updated controller account."
}
},
"description": "MsgUpdateControllerAccountResponse represents the response to an update\ncontroller account request."
},
"identityVerificationMethod": {
"type": "object",
"properties": {
"id": {
"type": "string",
+ "title": "The identifier for the verification method"
},
"type": {
"type": "string",
+ "title": "The type of the verification method"
},
"controller": {
"type": "string",
+ "title": "The entity controlling the verification method"
},
"publicKeyJwk": {
"type": "string",
+ "title": "Optional, the public key in JSON Web Key (JWK) format"
},
"publicKeyMultibase": {
"type": "string",
"title": "Optional, the public key in multibase-encoded format"
},
"blockchainAccountId": {
"type": "string",
+ "title": "Optional, the blockchain account identifier associated with the verification"
},
"attestationType": {
"type": "string",
+ "title": "Optional, the type of attestation associated with the verification"
},
"transports": {
"type": "array",
"items": {
"type": "string"
},
+ "title": "Optional, the set of transport protocols supported by the verification"
}
},
"description": "VerificationMethod represents a verification method that can be used to\nauthenticate the DID subject or perform other cryptographic operations."
},
"identityVerificationRelationship": {
"type": "object",
"properties": {
"verificationMethod": {
"$ref": "#/definitions/identityVerificationMethod",
+ "title": "The verification method associated with the relationship"
},
"reference": {
"type": "string",
+ "title": "The reference identifier for the relationship"
},
"type": {
"type": "string",
+ "title": "The type of the verification relationship"
},
"owner": {
"type": "string",
+ "title": "The idx address of the owner of the verification relationship"
}
},
"description": "VerificationRelationship represents a relationship between a verification\nmethod and a specific verification purpose (e.g., authentication, assertion,\netc.)."
},
"protobufAny": {
"type": "object",
"properties": {
"@type": {
"type": "string"
},
},
"additionalProperties": {}
},
"rpcStatus": {
"type": "object",
"properties": {
"code": {
"type": "integer",
"format": "int32"
}
}
}
These changes provide more clarity and improve the readability of the code.
"title": "core/service/record.proto", | ||
"version": "version not set" | ||
}, | ||
"consumes": [ | ||
"application/json" | ||
], | ||
"produces": [ | ||
"application/json" | ||
], | ||
"consumes": ["application/json"], | ||
"produces": ["application/json"], | ||
"paths": {}, | ||
"definitions": { | ||
"google.protobuf.Any": { | ||
"protobufAny": { | ||
"type": "object", | ||
"properties": { | ||
"type_url": { | ||
"@type": { | ||
"type": "string" | ||
}, | ||
"value": { | ||
"type": "string", | ||
"format": "byte" | ||
} | ||
} | ||
}, | ||
"additionalProperties": {} | ||
}, | ||
"grpc.gateway.runtime.Error": { | ||
"rpcStatus": { | ||
"type": "object", | ||
"properties": { | ||
"error": { | ||
"type": "string" | ||
}, | ||
"code": { | ||
"type": "integer", | ||
"format": "int32" |
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.
Sorry, it looks like something happened on our end and we failed to get feedback
This reverts commit 102f276. feat: fix package structure with did modules Remove/pkg cli deps (#815) * feat: remove cli pkgs * Revert "feat: remove cli pkgs" This reverts commit 858057e. * feat: swagger docs * feat: update lints --------- Signed-off-by: Prad Nukala <prad@sonr.io> fix: goreleaser status Revert "Remove/pkg cli deps (#815)" This reverts commit 43c28f8.
* Revert "feat: fix package structure with did modules" This reverts commit 102f276. feat: fix package structure with did modules Remove/pkg cli deps (#815) * feat: remove cli pkgs * Revert "feat: remove cli pkgs" This reverts commit 858057e. * feat: swagger docs * feat: update lints --------- Signed-off-by: Prad Nukala <prad@sonr.io> fix: goreleaser status Revert "Remove/pkg cli deps (#815)" This reverts commit 43c28f8. * Revert "Merge branch 'master' into revert/v0.8.0" This reverts commit 334d2fd, reversing changes made to 0bb2625. --------- Signed-off-by: Prad Nukala <prad@sonr.io>
No description provided.