-
Notifications
You must be signed in to change notification settings - Fork 1
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
local e2e demo with contract error propagation updates #6
Conversation
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.
Hey! Great job and typescript skills 👍
I just raised some minor things. Let's chat if you have some questions
cheqd-resource-uploader: | ||
image: ghcr.io/nymlab/cheqd-node:v2.0.1-arm64 | ||
ports: | ||
- target: 26656 |
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.
I think we don't need to expose ports here cause we do not run the node inside the container.
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.
updated in 7ce3c18
depends_on: | ||
- neutron-node | ||
ports: | ||
- target: 26656 |
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.
I assume here the same thing with exposing ports
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.
updated in 7ce3c18
avida-ts/package.json
Outdated
"ci:check": "pnpm --recursive run check", | ||
"setup": "pnpm run reset && pnpm install", | ||
"reset": "pnpm dlx rimraf --glob ./**/node_modules", | ||
"build": "NODE_ENV=production pnpm run --recursive --stream build", |
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.
Usually, if there are some jobs like build:txutils
or others it's assumed that build
action combines all the things together, like:
"build": "pnpm run build:txutils && pnpm run build:types && ..."
I think it worth to change build
job into something like build:production
or other thing which points to exact action.
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.
fixedin 7ce3c18
}, | ||
}; | ||
|
||
export async function get_sdjwt(privatePEM: string): Promise<SDJwtVcInstance> { |
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.
let's keep the same style, camelCase for functions.
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.
fixed in 7ce3c18
memo, | ||
}; | ||
|
||
let fee = await deployer.estimateFee(storeTx); |
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.
Are there any possible errors here could happen? Internet issues or others.
I think it worth at least to use try-catch
construction.
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.
fixed in 7ce3c18
@@ -0,0 +1,2 @@ | |||
// @see https://www.totaltypescript.com/ts-reset -- do not add any other lines of code to this file |
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.
As I remember, .d.ts
files are results of generating from .ts
files and they should not be placed in the repo cause build artifacts. Also, is it used somewhere?
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.
This is used in reset, admittedly not used too much in this code.
@@ -0,0 +1,75 @@ | |||
// @ts-check |
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 same issue here. Is it generated file and should it be placed in the repo?
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.
actually I am struggling with @ts-check for the avida-common-types at the moment in scripts/local-e2e-demo.mjs
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.
checks work in 7ce3c18
@@ -0,0 +1,37 @@ | |||
# Typescript library for AVIDA |
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.
Add building contract and schemas stages to doc
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.
added doc in 7ce3c18
ca9f87d
to
ea61dd1
Compare
4ed07d4
to
7ce3c18
Compare
VerifyResult
so that caller can handle appropriately but also easier to show erroravida-example
contract