-
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
Chore/client integration tests #759
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 5 Ignored Deployments
|
chainId: '0', | ||
guard: '5a2afbc4564b76b2c27ce5a644cab643c43663835ea0be22433b209d3351f937', | ||
}; | ||
export const targetAccount: IAccount = { |
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 is this one the same as sourceAccount?
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 opted for the usecase that an individual wants to transfer funds to his own account on another chain. I can easily update if it's more realistic to be a different account?
Nice job Ghislain! |
28aa26c
to
6b1957c
Compare
🦋 Changeset detectedLatest commit: 97849f8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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 need to update the github workflow file based on the new monorepo setup.
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.
Minor comments. Only static review
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.
Formatting needs to be executed. Dependencies aren't ordered alphabetically
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.
Should be done now.
let initialTargetBalance: number; | ||
|
||
describe('Cross Chain Transfer', () => { | ||
it('should fund the source account on chain 0', async () => { |
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.
it('should fund the source account on chain 0', async () => { | |
it('funds the source account on chain 0', async () => { |
|
||
// this is required as the source account is used to pay for gas; | ||
// in mainnet this is not required as the gas station pays for gas | ||
it('should fund the source account on chain 1', async () => { |
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.
it('should fund the source account on chain 1', async () => { | |
it('funds the source account on chain 1', async () => { |
expect(initialTargetBalance).toBeGreaterThanOrEqual(100); | ||
}); | ||
|
||
it('should be able to perform a cross chain transfer', async () => { |
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.
Etc....
it('should be able to perform a cross chain transfer', async () => { | |
it('performs a cross chain transfer', async () => { |
): Promise<number> { | ||
const tr = Pact.builder | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
.execution((Pact.modules as any).coin['get-balance'](account)) |
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.
Instead of using Pact.modules as any
you can write the Pact expression
.execution((Pact.modules as any).coin['get-balance'](account)) | |
.execution(`(coin.get-balance "${account}")`) |
(Pact.modules as any).coin['transfer-create']( | ||
sender00Account.account, | ||
receiver, | ||
readKeyset('ks'), | ||
amount, | ||
), |
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.
If you like this structure, generating the typings would be even better.
I think typings are available, considering the script
s in the packages/libs/client/package.json
"pactjs:generate:contract": "pactjs contract-generate --file contracts/coin.contract.pact",
"pactjs:retrieve:contract": "pactjs retrieve-contract --out contracts/coin.contract.pact --module coin",
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 tried making the typings available by running the first mentioned script (I had to slightly update it) this seems to generate typings but then using pact.modules.coin doesn't appear to work. I might need a tiny little bit of help getting that to work.
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 can't generate types for the contract here. The problem is that this file is in the client package, and we use interface merging for Pact types. This will break types in the pact.ts file itself. If we can somehow isolate this section, maybe with a separate tsconfig file, we might be able to fix it.
export function keyFromAccount(account: Account): string { | ||
return account.split(':')[1]; | ||
} |
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.
Taking the key from the account can only be done with k:...
accounts. Would be good to do a runtime check whether the account
is compatible. Even when it is, the key could've been rotated.
export function keyFromAccount(account: Account): string { | |
return account.split(':')[1]; | |
} | |
export function keyFromAccount(account: Account): string { | |
if (!account.includes('k:')) { | |
throw new Error(`Not able to retrieve key from '${account}'`); | |
} | |
return account.split(':')[1]; | |
} |
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.
@alber70g given that we provide a type for Account: export type Account =
${'k' | 'w'}:${string} | string;
Perhaps we could just return the account if there is no k:
prefix.
fb8e241
to
bd737cb
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.
Nitpicks only, good stuff ✨
packages/libs/client/TESTING.md
Outdated
[1]: https://www.docker.com/ | ||
[2]: /packages/tools/heft-rig/jest.config.json | ||
[3]: http://localhost:8080 | ||
|
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.
Bonus points if you add *.md
or TESTING.md
to the format:md
script in package.json
(and run that).
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.
It is done!
@@ -0,0 +1,10 @@ | |||
module.exports = { |
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.
Does it make sense to extend from packages/tools/heft-rig/jest.config.json
(and overwrite as needed like below), or not (yet)?
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 thought of that, but there are quite a few settings in the central config should not be used for the integration config. It might make sense to eventually add a shared integration config if more packages perform similar tests.
@@ -33,7 +33,8 @@ | |||
"pactjs:generate:contract": "pactjs contract-generate --file contracts/coin.contract.pact", | |||
"pactjs:retrieve:contract": "pactjs retrieve-contract --out contracts/coin.contract.pact --module coin", | |||
"start": "ts-node --transpile-only src/index.ts", | |||
"test": "jest" | |||
"test": "jest", | |||
"test:integration": "jest --silent -c ./jest.integration.config.js" |
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 go silent?
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.
Primarily to not output all the console logging done by Kadena Client. It makes the logs generated by Jest very hard to read.
<T extends any>(data: T): T => { | ||
console.log(tag, data); | ||
return data; | ||
}; |
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.
Idea: move to reusable helpers? Not necessarily in this PR.
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.
can do!
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 this is good enough and very valuable to merge it and have the test in the pipeline. Fixing the typing issues might require some revision of how we export contract types. For example, if we don't use interface merging, then we can easily generate types here.
…nning tests and determining coverage.
ca62c95
to
97849f8
Compare
This PR adds
test:integration
command. Invoke throughpnpm run test:integration