-
Notifications
You must be signed in to change notification settings - Fork 95
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
Conversation
size-limit report 📦
|
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be useful for polling strategy to be an option?
methodName: string; | ||
agent: Agent; | ||
arg: ArrayBuffer; | ||
}): Promise<ArrayBuffer> { |
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 get back the contentMap
and the rawCert
. Ideally, this function would have the exact same interface as call
, maybe with the addition of being able to supply the pollStrategy
as suggested by @dfx-json.
Also, please extend the CallOptions
interface to allow specifying the nonce
too.
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.
@frederikrothenberger can you define what contentMap
precisely is? Should it be an ArrayBuffer? Does it need to be decoded into a JavaScript object? Are you willing to provide an IDL interface for the types so we can actually decode the candid?
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
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.
A few comments in addition to those of @frederikrothenberger by comparing with what we have implemented in the Oisy Wallet Signer library.
Source: https://github.com/dfinity/oisy-wallet-signer/blob/main/src/agent/custom-http-agent.ts
}; | ||
|
||
const certificate = await callAndPoll(options); | ||
expect(certificate instanceof ArrayBuffer).toBe(true); |
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!
}); | ||
|
||
let certificate: Certificate; | ||
if (response.body && (response.body as v3ResponseBody).certificate) { |
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 you assert that the type is v3 before casting? This feels a bit optimistic.
if (response.body && (response.body as v3ResponseBody).certificate) { | ||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the certificate status
be asserted at this point?
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 wasn't (and still won't be until tomorrow) sure what the intended purpose for this feature is. Certificate.create
will validate the certificate, but not check the status. There's no problem with throwing if it's rejected, but I wasn't sure if that was desireable behavior for this low-level feature
certificate = response.certificate; | ||
} | ||
|
||
return certificate.rawCert; |
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
The throw
for an invalid response will originate from the Certificate.create
step, either in the sync or polling flow. I could capture it and throw a custom error here for code legibility purposes
Description
Adds a new utility -
callAndPoll
. This function allows you to make a call using theHttpAgent
and retrieving the rawcertificate
, either using the v3 sync call or falling back to polling. This duplicates the behavior done byActor
at a low level, and is intended to be used by other libraries.Fixes # SDK-1828
How Has This Been Tested?
New e2e test
Checklist: