-
Notifications
You must be signed in to change notification settings - Fork 285
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
feat(daml): creation of connector class #3615
base: main
Are you sure you want to change the base?
feat(daml): creation of connector class #3615
Conversation
886560c
to
cd091b0
Compare
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.
Please incorporate these changes and then re-request for review (Make sure to squash the commits into a single commit)
"generate-sdk:go": "openapi-generator-cli generate -i ./src/main/json/openapi.json -g go -o ./src/main/go/generated/openapi/go-client/ --git-user-id hyperledger --git-repo-id $(echo $npm_package_name | replace @hyperledger/ \"\" -z)/src/main/go/generated/openapi/go-client --package-name $(echo $npm_package_name | replace @hyperledger/ \"\" -z) --reserved-words-mappings protected=protected --ignore-file-override ../../openapi-generator-ignore", | ||
"generate-sdk:kotlin": "openapi-generator-cli generate -i ./src/main/json/openapi.json -g kotlin -o ./src/main/kotlin/generated/openapi/kotlin-client/ --reserved-words-mappings protected=protected --ignore-file-override ../../openapi-generator-ignore", | ||
"generate-sdk:typescript-axios": "openapi-generator-cli generate -i ./src/main/json/openapi.json -g typescript-axios -o ./src/main/typescript/generated/openapi/typescript-axios/ --ignore-file-override ../../openapi-generator-ignore", | ||
"generate-server": "yarn run --top-level openapi-generator-cli generate -i ./src/main/json/openapi.json -g kotlin-spring -o ./src/main-server/kotlin/gen/kotlin-spring/ -c ./src/main-server/openapi-generator-config.yaml --ignore-file-override ../../openapi-generator-ignore", |
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 do not need this. Please remove this (might got have remained when you copied this from corda connector)
} | ||
}, | ||
"operationId": "exerciseChoice", | ||
"summary": "This method creates a simple iou countract", |
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.
Please update the summary and the following description of this path
} | ||
}, | ||
"operationId": "queryRawContract", | ||
"summary": "This method queries DAML contracts", |
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.
Please update this, along with the description and mention why is it different from the above query-iou-endpoint
method
} | ||
}, | ||
"operationId": "getPartiesInvolved", | ||
"summary": "This method queries DAML contracts", |
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.
Please update this to match the referenced path
import type { Server as SecureServer } from "https"; | ||
import type { Config as SshConfig } from "node-ssh"; | ||
import type { Express } from "express"; | ||
// import urlcat from "urlcat"; |
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.
Please remove all instances of uncommented (unused) code.
try { | ||
if (this.apiUrl === undefined) | ||
throw new InternalServerError("apiUrl option is necessary"); | ||
const body = await this.options.connector.createContract(req.body, "testdata123"); |
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 we passing testdata123
here? Should this variable be passed from the test case itself if required
? (And thus be a part of req.body
?)
try { | ||
if (this.apiUrl === undefined) | ||
throw new InternalServerError("apiUrl option is necessary"); | ||
const body = await this.options.connector.exerciseContract(req.body, "testdata123"); |
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 we passing testdata123 here? Should this variable be passed from the test case itself if required
? (And thus be a part of req.body?)
} | ||
|
||
async handleRequest(req: Request, res: Response): Promise<void> { | ||
const fnTag = "QueryIOURawEndpointRequest#handleRequest()"; |
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.
Please update this to QueryRawIOUEndpoint...
to match the api endpoint
} | ||
const createIou = await apiClient.createIou(iouBody) | ||
console.log("STEP 1. Create IOU as Alice result:") | ||
console.log(createIou) |
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.
Use LoggerProvider class from cactus-common instead
responseForIOUResult = JSON.parse(stringifyIOUPayload) | ||
|
||
|
||
console.log("STEP 4. Check if transfer is successful by querying as BOB") |
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.
Please rephrase all these statements (along with using LoggerProvider class). Right now it looks like this is the line from where step4 execution starts.
cd091b0
to
20cc75d
Compare
} | ||
|
||
public async transact(): Promise<any> { | ||
return null as 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.
@jagpreetsinghsasan I agree, this looks closer to something application specific instead of belong in the generic framework code. I'm not an expert on DAML/Canton, but maybe we are looking for something like this instead?
https://docs.daml.com/app-dev/grpc/proto-docs.html#submitandwait-method-version-com-daml-ledger-api-v1
1a99027
to
5b9e094
Compare
Hello @petermetz , May I request for you to update the docker image of this task?: Cheers! Rayn |
5b9e094
to
0e0ae2e
Compare
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.
@raynatopedrajeta Please don't forget to fix the custom-checks that are failing:
[tools/check-missing-node-deps.ts] ERROR - MISSING production dependency node-fetch from /********/packages/cactus-plugin-ledger-connector-daml/package.json. Found usage in
/********/packages/cactus-plugin-ledger-connector-daml/src/main/typescript/plugin-ledger-connector-daml.ts
[tools/check-missing-node-deps.ts] ERROR - MISSING production dependency express from /********/packages/cactus-plugin-ledger-connector-daml/package.json. Found usage in
/********/packages/cactus-plugin-ledger-connector-daml/src/main/typescript/web-services/create-iou-endpoint.ts
/********/packages/cactus-plugin-ledger-connector-daml/src/main/typescript/web-services/exercise-iou-endpoint.ts
/********/packages/cactus-plugin-ledger-connector-daml/src/main/typescript/web-services/get-parties-involved-endpoint.ts
/********/packages/cactus-plugin-ledger-connector-daml/src/main/typescript/web-services/query-iou-endpoint.ts
/********/packages/cactus-plugin-ledger-connector-daml/src/main/typescript/web-services/query-raw-iou-endpoint.ts
"node-fetch" is a missing dependency from /********/packages/cactus-plugin-ledger-connector-daml/package.json with the following files using them:
/********/packages/cactus-plugin-ledger-connector-daml/src/main/typescript/plugin-ledger-connector-daml.ts
Primary Changes --------------- 1. Create a DAML connector class 2. Created OpenAPI endpoints of DAML 3. Created DAML web services for create, exercise and query contracts 4. Create simple IOU Transaction using DAML Fixes hyperledger-cacti#3489 Co-authored-by: Peter Somogyvari <peter.somogyvari@accenture.com> Signed-off-by: raynato.c.pedrajeta <raynato.c.pedrajeta@accenture.com> Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
0e0ae2e
to
67919b6
Compare
@raynatopedrajeta I tried to publish the image but it fails to upload because one of the layers is 1.5GB and the upload keeps retrying to infinity. Could you please reduce the layer size by splitting the commands in the dockerfile? I'm pretty sure it's this one: # Download and install DAML SDK 2.9.3
RUN curl -L https://github.com/digital-asset/daml/releases/download/v2.9.3/daml-sdk-2.9.3-linux.tar.gz | tar -xz -C /opt && \
cd /opt/sdk-2.9.3 && \
./install.sh && \
rm -rf /opt/sdk-2.9.3/ |
@raynatopedrajeta FYI: I pushed a small change to modify the readme of the DAML aio image so that it's easier to build the image in the future for publishing: ## Build an image locally
To build the daml-all-in-one image locally, use:
```sh
DOCKER_BUILDKIT=1 docker build \
--file ./tools/docker/daml-all-in-one/Dockerfile \
./tools/docker/daml-all-in-one/ \
--tag daio \
--tag daml-all-in-one \
--tag ghcr.io/hyperledger/daml-all-in-one:$(date +"%Y-%m-%dT%H-%M-%S" --utc)-dev-$(git rev-parse --short HEAD)
|
Commit to be reviewed
feat(daml): creation of connector class
Fixes #3489
Pull Request Requirements
upstream/main
branch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.-s
flag when usinggit commit
command. You may refer to this link for more information.Character Limit
A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.