-
Notifications
You must be signed in to change notification settings - Fork 97
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: call and poll #941
base: main
Are you sure you want to change the base?
feat: call and poll #941
Changes from 3 commits
9d0ab85
b4a90fb
d983a95
a559435
059905a
5befbdb
7b52f29
8843eeb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
import { HttpAgent, fromHex, callAndPoll } from '@dfinity/agent'; | ||
import { expect, describe, it, vi } from 'vitest'; | ||
describe('call and poll', () => { | ||
it('should handle call and poll', async () => { | ||
vi.useRealTimers(); | ||
|
||
const options = { | ||
canisterId: 'tnnnb-2yaaa-aaaab-qaiiq-cai', | ||
methodName: 'inc_read', | ||
agent: await HttpAgent.create({ host: 'https://icp-api.io' }), | ||
arg: fromHex('4449444c0000'), | ||
}; | ||
|
||
const certificate = await callAndPoll(options); | ||
expect(certificate instanceof ArrayBuffer).toBe(true); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
import { Principal } from '@dfinity/principal'; | ||
import { Agent, Certificate, bufFromBufLike, polling, v3ResponseBody } from '..'; | ||
import { AgentError } from '../errors'; | ||
|
||
/** | ||
* Call a canister using the v3 api and either return the response or fall back to polling | ||
* @param options - The options to use when calling the canister | ||
* @param options.canisterId - The canister id to call | ||
* @param options.methodName - The method name to call | ||
* @param options.agent - The agent to use to make the call | ||
* @param options.arg - The argument to pass to the canister | ||
* @returns The certificate response from the canister (which includes the reply) | ||
*/ | ||
export async function callAndPoll(options: { | ||
canisterId: Principal | string; | ||
methodName: string; | ||
agent: Agent; | ||
arg: ArrayBuffer; | ||
}): Promise<ArrayBuffer> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to get back the Also, please extend the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @frederikrothenberger can you define what Relatedly, I don't know what actual feature this is for, or how it will be used in practice. We'd probably have had a lot less back-and-forth about this if I was just designing a feature for your use-case instead of all these half-requirements and us each having slightly different definitions of what things are |
||
const { canisterId, methodName, agent, arg } = options; | ||
const cid = Principal.from(options.canisterId); | ||
|
||
const { defaultStrategy } = polling.strategy; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be useful for polling strategy to be an option? |
||
|
||
if (agent.rootKey == null) throw new Error('Agent root key not initialized before making call'); | ||
|
||
const { requestId, response } = await agent.call(cid, { | ||
methodName, | ||
arg, | ||
effectiveCanisterId: cid, | ||
}); | ||
|
||
let certificate: Certificate; | ||
if (response.body && (response.body as v3ResponseBody).certificate) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should you assert that the type is v3 before casting? This feels a bit optimistic.
krpeacock marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const cert = (response.body as v3ResponseBody).certificate; | ||
// Create certificate to validate the responses | ||
certificate = await Certificate.create({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't the certificate There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wasn't (and still won't be until tomorrow) sure what the intended purpose for this feature is. |
||
certificate: bufFromBufLike(cert), | ||
rootKey: agent.rootKey, | ||
canisterId: Principal.from(canisterId), | ||
}); | ||
} else { | ||
throw new AgentError('unexpected call error: no certificate in response'); | ||
} | ||
// Fall back to polling if we recieve an Accepted response code | ||
if (response.status === 202) { | ||
const pollStrategy = defaultStrategy(); | ||
// Contains the certificate and the reply from the boundary node | ||
const response = await polling.pollForResponse(agent, cid, requestId, pollStrategy); | ||
certificate = response.certificate; | ||
} | ||
|
||
return certificate.rawCert; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't it throws if none of the above was a valid response? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
} |
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 you going to enhance the tests? e.g. asserting that the certificate return is the expected value or asserting that the function correctly throws if there was an error?
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.
Yes, this should have been marked as a draft. I wanted feedback on the inputs and return values before proceeding. If you have real-world test cases this feature will be used for, those would be great to add as tests as well, beyond the basic cases!