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!: support getting certificate back from call #892

Merged
merged 36 commits into from
Jun 26, 2024

Conversation

krpeacock
Copy link
Contributor

@krpeacock krpeacock commented Jun 11, 2024

Description

In order to support ICRC-49, we need the HttpAgent to provide more details back after making a call. With this change, HttpAgent.call will provide:

requestId - the computed request ID to poll for
response - the raw `http` response from the boundary node
requestDetails - the details sent to the canister, used to compute the request ID

In addition, the output from pollForResponse needs to be updated as well. PollForResponse now returns

certificate: the Certificate tree sent along with the reply
reply: the certified response from the replica

This will be a breaking change, requiring a major version bump.

Checklist:

  • My changes follow the guidelines in CONTRIBUTING.md.
  • The title of this PR complies with Conventional Commits.
  • I have edited the CHANGELOG accordingly.
  • I have made corresponding changes to the documentation.

@krpeacock krpeacock changed the title Kai/sdk 1717 raw call feat: support getting certificate back from call Jun 11, 2024
Copy link
Contributor

github-actions bot commented Jun 11, 2024

size-limit report 📦

Path Size
@dfinity/agent 85.94 KB (+0.21% 🔺)
@dfinity/candid 13.58 KB (0%)
@dfinity/principal 4.97 KB (0%)
@dfinity/auth-client 60.8 KB (0%)
@dfinity/assets 80.6 KB (+0.16% 🔺)
@dfinity/identity 58 KB (0%)
@dfinity/identity-secp256k1 266.45 KB (+0.12% 🔺)

@krpeacock
Copy link
Contributor Author

@frederikrothenberger @peterpeterparker with these changes, you can forward a call without being aware of the arguments using code that would look something like this:

  const forwardedOptions = {
    canisterId: 'tnnnb-2yaaa-aaaab-qaiiq-cai',
    methodName: 'inc_read',
    arg: '4449444c0000',
    effectiveCanisterId: 'tnnnb-2yaaa-aaaab-qaiiq-cai',
  };

  const agent = new HttpAgent({ host: 'https://icp-api.io', fetch: global.fetch });
  const { requestId, response, requestDetails } = await agent.call(
    Principal.fromText(forwardedOptions.canisterId),
    {
      methodName: forwardedOptions.methodName,
      arg: fromHex(forwardedOptions.arg),
      effectiveCanisterId: Principal.fromText(forwardedOptions.effectiveCanisterId),
    },
  );

  const { certificate, reply } = await pollForResponse(
    agent,
    Principal.fromText(forwardedOptions.effectiveCanisterId),
    requestId,
    defaultStrategy(),
  );
  certificate; // Certificate
  reply; // ArrayBuffer

Should I wrap all of this functionality into a callRaw method, or does this satisfy your requirements?

Nonce generation is not used in calls and is disabled by default for queries with the agent now, but you will be able to control it through the useQueryNonces setting or adding transforms to the agent.

You can inspect any nonces as well as the ingress_expiry, as these are now returned by agent.call inside of requestDetails

@frederikrothenberger
Copy link
Contributor

@krpeacock: Yes, this satisfies our requirement. Thanks a lot.

@dostro: Please share this with your team / compare to your fork and also provide feedback. Thanks!

@krpeacock krpeacock marked this pull request as ready for review June 14, 2024 23:37
@krpeacock krpeacock requested a review from a team as a code owner June 14, 2024 23:37
Copy link
Contributor

@frederikrothenberger frederikrothenberger left a comment

Choose a reason for hiding this comment

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

Thanks a lot, this is what we need. 👍
@peterpeterparker: Want to take a look as well?

packages/agent/src/agent/http/http.test.ts Outdated Show resolved Hide resolved
@sea-snake
Copy link
Contributor

Related to this, on my end with signer-js, I need to create a CallRequest instance but have to work around the fact that I can't create an Expiry instance with a specific value since this value is private and the constructor adds the current time to the passed argument.

Current workaround in signer-js:

const expiry = new Expiry(0);
// @ts-ignore Expiry class currently has no method to create instance from value
expiry._value = /* Some BigInt value */

Would be nice to be able to create an Expiry instance with a specific value or simply be able to modify it's value 😅

@dmitriiIdentityLabs
Copy link

Thank you, @frederikrothenberger, for letting us know about the PR!

Context: While implementing the ICRC-49 call canister standard, I faced challenges making the canister call on behalf of the user with the current agent.js implementation. To address this, I created a new actor, similar to ActorWithHttpDetails, called CallCanisterActor. Its sole purpose is to be useful for signer apps.

I need 4 items for that:

  1. Certificate.
  2. ContentMap.
  3. Update call enforcement.
  4. Ability to pass candid-encoded arguments to the actor.

Regarding the certificate, everything seems fine in the PR.

What's missing:

  1. There is no ContentMap as an output. I return contentMap: transformedRequest.body.content in packages/agent/src/agent/http/index.ts. Potentially, I can rebuild it outside of agent.js, but it requires the application of all inner transformations.
  2. The signer calls are always update calls, even if they are queries in Candid. To achieve this, I enforce agent.call if I have my option in the annotations: func.annotations.includes('call-canister').
  3. The signer call arguments are always candid-encoded before the actor method. There is no need to encode them in an actor. Therefore, I implement a different workflow to prevent the encoding of the argument if I have my option in the annotation: func.annotations.includes('call-canister').

@frederikrothenberger, are there better ways to achieve these three objectives without changing agent.js? Shall we consider the PR to cover all the cases?

@sea-snake
Copy link
Contributor

sea-snake commented Jun 19, 2024

To address this, I created a new actor, similar to ActorWithHttpDetails, called CallCanisterActor.

Why would you need an actor to make a call with raw arguments?

To clarify, the HttpAgent allows you to make communicate with canister methods annotated as either call, query (or composite-query). While an actor additionally simplifies things by using the canister candid definition to automatically create a class with methods for all canister methods defined in candid. An actor also automatically decides if it should use either a call or make a query for each candid method, automatically converts JS arguments into a single binary argument etc.

In the Signer standards, the decision was made to always call canister methods even if they could be queried instead. This decision was made due to various security aspects. Also the received arguments are already encoded by the relying party, so there's no need for an Actor to encode anything.

In practice this means that you don't need to create an Actor to make a ICRC-49 canister call since:

  • You don't need a js instance to represent a canister instance
  • You always want to make a call, not a query
  • The arguments are already encoded, so encoding based on candid definition isn't needed

Given the incoming JSON-RPC message:

{
    "id": 1,
    "jsonrpc": "2.0",
    "method": "icrc49_call_canister",
    "params": {
        "canisterId": "xhy27-fqaaa-aaaao-a2hlq-ca",
        "sender": "b7gqo-ulk5n-2kpo7-oalt7-p2kyl-o4j5l-kiuwo-eeybr-dab4l-ur6up-pqe",
        "method": "transfer",
        "arg": "RElETARte24AbAKzsNrDA2ithsqDBQFs5ofQIAAMgB"
    }
}

The signer could use below code to call the canister and return a JSON-RPC response:

import { HttpAgent, Cbor, polling} from '@dfinity/agent';
import { Buffer } from 'buffer';

const { params } = rpcRequest; // The JSON-RPC request from above
const agent = new HttpAgent();
const { pollForResponse, defaultStrategy } = polling;
const { requestId, requestDetails } = await agent.call(
    Principal.fromText(params.canisterId),
    {
      methodName: params.methodName,
      arg: Buffer.from(params.arg, 'base64').buffer,
    },
);

const contentMap = Cbor.encode(requestDetails);
const { certificate } = await pollForResponse(
    agent,
    Principal.fromText(params.canisterId),
    requestId,
    defaultStrategy(),
);

// JSON-RPC response for above JSON-RPC request
const rpcResponse = {
    id: rpcRequest.id,
    jsonrpc: '2.0',
    result: {
       contentMap: Buffer.from(contentMap).toString('base64'),
       certificate: Buffer.from(certificate).toString('base64'),
    }
};

@krpeacock Not having the cbor exported and available from @dfinity/agent or another lib is cumbersome though, currently I have to manually copy its implementation into my own project.

If you want to decode the already encoded binary argument and visualize it in the signer, that's a whole different story. You'd need to fetch the candid from the canister metadata and use that to decode the binary call argument into JS variables and then visualize this JS by e.g. rendering a tree. Overall, ideally you'd like to avoid the need for this and rely on ICRC-21 instead for human readable information to be presented to the end user.

@krpeacock
Copy link
Contributor Author

@sea-snake We've always had the cbor implementation exported -

import { Cbor } from '@dfinity/agent';

Am I missing something there?

@sea-snake
Copy link
Contributor

sea-snake commented Jun 20, 2024

Am I missing something there?

Wasn't aware of this export, great to know!

@krpeacock krpeacock changed the title feat: support getting certificate back from call feat!: support getting certificate back from call Jun 20, 2024
@dmitriiIdentityLabs
Copy link

@sea-snake Thank you for the excellent example! I'll use it if we decide not to change the actor. The main issue is that the signer has to reuse a lot of code from the actor for polling, and encoding. This code could change, meaning every signer would need to implement it, placing the burden of compliance on the signer's shoulders.

The ideal scenario for me would be:

const actor = SignerActor.create(candidjs, {agent, canisterId});
const {contentMap, certificate} = await actor[method](encodedArg);

Why would you need an actor to make a call with raw arguments?

There's a way to avoid reimplementing the actor and copying its code. We could create a specific SignerActor class designed solely for signer call canisters, adhering strictly to the standards.

If you want to decode the already encoded binary argument and visualize it in the signer, that's a whole different story. You'd need to fetch the candid from the canister metadata and use that to decode the binary call argument into JS variables and then visualize this JS by e.g. rendering a tree. Overall, ideally you'd like to avoid the need for this and rely on ICRC-21 instead for human readable information to be presented to the end user.

Our example signer already implements this. We fetch the candid to decode arguments and display them to the user. I understand your point, but I worry we cannot fully rely on ICRC-21 for displaying human-readable data to users. The standard might not be implemented, and the signer might require additional validation for known call canister requests. Therefore, the signer must understand the data. It would also be beneficial to simplify the signer's task by providing a default way to fetch and process the candid dynamically.

@frederikrothenberger @sea-snake @krpeacock
The main question is: Is there any chance we could consider adding a specific actor for call canister to agent-js, or would it be better to postpone this?

@krpeacock
Copy link
Contributor Author

@dmitriiIdentityLabs I can add that, but I think I'll address it in a separate PR. I'm fairly sure that this API with these breaking changes will give us all we need to build that actor, so it can be added in a future PR with just a minor version bump

@sea-snake
Copy link
Contributor

The ideal scenario for me would be:

const actor = SignerActor.create(candidjs, {agent, canisterId});
const {contentMap, certificate} = await actor[method](encodedArg);

I would at least not call it an actor since it isn't an actor in comparison to what actors currently are (following the actor model).

Not sure what the candidjs argument is for, there's no candid involved in either the input or output (both are already encoded raw bytes.

Also keep in mind that the ICRC-49 spec is working with raw already encoded bytes as input/output intentionally, because technically nothing stops a canister from using something other than candid for it's input/output. So not relying on candid in ICRC-49 keeps it future compatible with any data format.

@krpeacock krpeacock merged commit 972e3cb into main Jun 26, 2024
16 checks passed
@krpeacock krpeacock deleted the kai/SDK-1717-raw-call branch June 26, 2024 22:53
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.

5 participants