Skip to content

fix proxy tests#398

Closed
xlc wants to merge 1 commit intomasterfrom
fix-proxy
Closed

fix proxy tests#398
xlc wants to merge 1 commit intomasterfrom
fix-proxy

Conversation

@xlc
Copy link
Copy Markdown
Member

@xlc xlc commented Sep 9, 2025

fix proxy e2e tests

@xlc xlc requested a review from rockbmb September 9, 2025 08:03
await check(announcements[0][0]).toMatchObject(announcementObject)
await check(announcements[0][0]).toMatchObject({
...announcementObject,
height: currBlockNumber + 4,
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this is not exactly right

@xlc
Copy link
Copy Markdown
Member Author

xlc commented Sep 9, 2025

@rockbmb would you be able to continue on this? also not exactly sure what's wrong but running those tests locally was very very slow for me

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

The changes introduce a new useRelayBlockNumber parameter to handle differences in block numbering between parachains and relay chains in proxy-related tests. However, this new logic is applied incorrectly in two places, likely causing test failures.

// `createPure` extrinsics were executed.
const currBlockNumber = (await client.api.rpc.chain.getHeader()).number.toNumber()

const currBlockNumber = await getBlockNumber(client, useRelayBlockNumber)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In createKillPureProxyTest, the currBlockNumber is fetched using getBlockNumber, which may return a relay chain block number. This block number is then passed to the proxy.killPure extrinsic. The killPure extrinsic requires the block number of the parachain where the proxy was created, not a relay chain block number. Using the relay block number will cause the extrinsic to fail as it won't be able to find the proxy's creation event.

Comment on lines +1848 to +1851
await check(announcements[0][0]).toMatchObject({
...announcementObject,
height: currBlockNumber + 4,
})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In proxyAnnouncementLifecycleTest, the expected block height for a re-announced proxy call is hardcoded to currBlockNumber + 4. This offset is likely incorrect for tests that do not use the relay block number (i.e., when useRelayBlockNumber is false), where the expected offset should be +2. Applying the +4 offset unconditionally will break these tests. The offset should be determined based on the value of useRelayBlockNumber.

@rockbmb
Copy link
Copy Markdown
Collaborator

rockbmb commented Sep 9, 2025

@xlc I can take over.

})
}

export async function getBlockNumber(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I also had to do this in #384 .

/**
* Get the last known block number for a given chain.
*
* The block provider might be local or external (e.g. a parachain querying its relay chain).
*
* @param api Promise-based RPC wrapper around the endpoint of a Polkadot chain.
* @returns The last known block number if relay, the relay chain block number the most recent parablock was anchored
* to if parachain.
*/
export async function getBlockNumber(api: ApiPromise, blockProvider: BlockProvider): Promise<number> {
return await match(blockProvider)
.with('Local', async () => (await api.rpc.chain.getHeader()).number.toNumber())
.with('NonLocal', async () => ((await api.query.parachainSystem.lastRelayChainBlockNumber()) as any).toNumber())
.exhaustive()
}

@rockbmb
Copy link
Copy Markdown
Collaborator

rockbmb commented Sep 10, 2025

#384 fixes the proxy tests' differing announcement block numbers, so closing in favor of it.

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.

2 participants