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

feat: add basic integration tests #2

Merged
merged 22 commits into from
Dec 1, 2023
Merged

feat: add basic integration tests #2

merged 22 commits into from
Dec 1, 2023

Conversation

ntn-x2
Copy link
Member

@ntn-x2 ntn-x2 commented Nov 30, 2023

Partially fixes https://github.com/KILTprotocol/ticket/issues/2975.

Basic integration tests setup based on Zombienet and Kubernetes provider. Currently there is no easy way (or at least I have not found any) to run integration tests in the CI in a predictable way to have access to the ports forwarded for the nodes. Probably there is a way around that, but for now I'd merge this PR (which has helped finding out a bunch of errors in the code) and move on. I also removed the parent stuff as that is less easy to test (requires a custom Polkadot image with the consumer pallet), and while I have verified elsewhere that it works, I will focus on this also later on. What brings the most value is the sibling bridging, which this PR enables.

@ntn-x2 ntn-x2 self-assigned this Nov 30, 2023
Copy link
Contributor

@rflechtner rflechtner left a comment

Choose a reason for hiding this comment

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

Looks good, no complaints!

.github/workflows/build.yaml Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
* @returns A fresh instance of an [[ObjectBuilder]].
*/
export function dipSiblingProofBuilder(): ObjectBuilder {
return ObjectBuilder.new<DipSiblingProofInput>()
Copy link
Contributor

Choose a reason for hiding this comment

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

is the object builder not helping?

Copy link
Member Author

@ntn-x2 ntn-x2 Nov 30, 2023

Choose a reason for hiding this comment

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

I was having some weird TS issues, since the class is exported by the IWith interface on which the class is based is not, and even when I tried to use it in the e2e tests, it was more of a hussle to define a base config and then extend it based on each test case, since all it does is TS magic. I decided to opt out of it, and instead use the config strategy you see in the e2e tests, which is much more ergonomic.

src/sibling.ts Show resolved Hide resolved
src/utils.ts Show resolved Hide resolved

const okProof = proof.unwrap() as any
// TODO: Better way to cast this?
const okProof = proof.asOk.toJSON() as any
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const okProof = proof.asOk.toJSON() as any
const okProof = proof.asOk.toJSON() as DipIdentityProofRes

?

Copy link
Member Author

@ntn-x2 ntn-x2 Nov 30, 2023

Choose a reason for hiding this comment

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

Conversion of type 'string | number | boolean | AnyJson[] | { [index: string]: AnyJson; } | null | undefined' to type 'DipIdentityProofRes' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
  Type '{ [index: string]: AnyJson; }' is missing the following properties from type 'DipIdentityProofRes': proof, root ts(2352)

Copy link
Member Author

Choose a reason for hiding this comment

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

toJSON() returns AnyJSON, which is not compatible with anything, basically.

src/utils.ts Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
tests/utils.ts Outdated
RegistryTypes,
} from "@polkadot/types/types"

const dipProviderCalls: DefinitionsCall = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this not something the dip-sdk should just expose? Don't you need that to create an api that connects to a provider chain?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. While the types are provider-specific, and here they refer to the provider template runtime, the runtime definitions can be exported, so I did just that. What do you think? afc7624

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah makes sense to me. Are there any types on the provider side which are fixed simply because the proof would not verify if you changed them? If so, they could be exposed here as well I guess?

Copy link
Member Author

@ntn-x2 ntn-x2 Nov 30, 2023

Choose a reason for hiding this comment

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

Mhh the whole identity proof is generic over pretty much everything, so there's nothing more that could be assumed to be part of the provider runtime. The only thing that is fixed is the IdentityCommitmentVersion: "u16" type. Should I export just that? Even the block number and account ID are generic, and I've hit that very same issue myself since Peregrine uses a u64 for the block number and the provider runtime a u32. This whole thing will anyway become outdated once metadata v15 is out.

Copy link
Member

@Dudleyneedham Dudleyneedham left a comment

Choose a reason for hiding this comment

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

LGTM, some minor comments.

.github/workflows/build.yaml Outdated Show resolved Hide resolved
src/sibling.ts Show resolved Hide resolved
@ntn-x2 ntn-x2 merged commit 089bda9 into main Dec 1, 2023
1 check passed
@Dudleyneedham Dudleyneedham deleted the aa/integration-tests branch December 4, 2023 08:51
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.

3 participants