decentralized-identity / veramo

A JavaScript Framework for Verifiable Data
https://veramo.io
Apache License 2.0
414 stars 130 forks source link

cli `veramo execute ...` fails with `Maximum call stack size exceeded` #1281

Closed pauldesmondparker closed 4 months ago

pauldesmondparker commented 8 months ago

Bug severity 4

Describe the bug

veramo execute -m resolveDid ...

Results in:

Maximum call stack size exceeded

I'm fairly confident my usage is correct due to: https://github.com/decentralized-identity/veramo/issues/681 and this code:

did
  .command('resolve <didUrl>')
  .description('Resolve DID Document')
  .action(async (didUrl: string, opts: {}, cmd: Command) => {
    const agent = await getAgent(cmd.optsWithGlobals().config)
    try {
      const ddo = await agent.resolveDid({ didUrl })  // <<----- Usage here.
      console.log(JSON.stringify(ddo, null, 2))
    } catch (e: any) {
      console.error(e.message)
    }
  })

To Reproduce Steps to reproduce the behaviour:

  1. Add ens did-method to agent.yml:
      ens:
        $ref: /universal-resolver
  2. Calling:

    veramo execute -m resolveDid -a '{"didUrl": "did:ens:vitalik.eth"}'

    Fails.

Observed behaviour Maximum call stack size exceeded

Expected behaviour DID resolution. The same as the result of:

veramo did resolve did:ens:vitalik.eth

Which works fine.

Details image

Versions:

pauldesmondparker commented 8 months ago

Try directly with next branch and leveraging --stack-size:

node --stack-size=8000 ./packages/cli/bin/veramo.js execute -m packDIDCommMessage -a '{"packing":"anoncrypt","message":{"to":"did:key:z6MktEQbgrewCxg3bXkdKAqHJXSEMJVcxUhcEvkWVqyBpzYn", "body":{"hello":"world"}}}'

Still results in the same error. --stack-size=9000 results in a segfault on my machine.

lampkin-diet commented 8 months ago

Have the same error but while using -m keyManagerCreate:

veramo execute -m keyManageCreate
Maximum call stack size exceeded

veramo version: 5.5.3 Node version: node --version v18.17.1

pauldesmondparker commented 8 months ago

This is a result of the following line in packages/cli/src/execute.ts

      const { openapi } = await OasResolver.resolve(agent.getSchema(), null, { resolveInternal: true })

The OasResolver.resolve does use a recursive method to traverse the schema, so the error makes sense. However, there is no mention of Maximum call stack size exceeded in the issues of the github repository for oas-resolver.

My working theory at this stage is that there is a cycle in the schema that the OpenApi Schema resolver (oas-resolver) cannot handle.

pauldesmondparker commented 8 months ago

I have gotten veramo execute working, but it required modification of oas-resolver code.

The following works:

node ./packages/cli/bin/veramo.js execute -m resolveDid -a '{"didUrl": "did:ens:vitalik.eth"}'

If this change is made to oas-resolver:

diff --git a/packages/oas-resolver/index.js b/packages/oas-resolver/index.js
index ab2ce37..655d505 100644
--- a/packages/oas-resolver/index.js
+++ b/packages/oas-resolver/index.js
@@ -118,6 +118,7 @@ function resolveAllFragment(obj, context, src, parentPath, base, options) {
     recurse(obj,{},function(obj,key,state){
         if (isRef(obj, key)) {
             if (typeof obj.$fixed !== 'undefined') delete obj.$fixed;
+            if (!options.preserveMiro) delete obj['x-miro'];
         }
     });

@@ -466,11 +467,11 @@ function loopReferences(options, res, rej) {
                                 options.openapi = deRef(options.openapi,options.original,{verbose:options.verbose-1});
                                 if (options.verbose>1) console.warn(common.colour.yellow+'Finished internal resolution!',common.colour.normal);
                             }
-                            recurse(options.openapi,{},function(obj,key,state){
-                                if (isRef(obj, key)) {
-                                    if (!options.preserveMiro) delete obj['x-miro'];
-                                }
-                            });
                             res(options);
                         }
                     }

Which is undoing commit: aabd716b42ebac8b75135db8575725ee66867850 made on Mon Feb 18 16:59:59 2019 +0000.

I have no idea why, after many other calls to recurse on options.openapi work fine in the process of completing loopReference(...), that this one fails for the veramo schema, but it does.

I cloned oas-kit locally, modified oas-resolver, and then made it a local dependency in the main package.json and the cli/package.json.

    "oas-resolver": "file:../oas-kit/packages/oas-resolver",
pauldesmondparker commented 8 months ago

The above commit is necessary to solve another issue. However, I found a simpler solution that just changes a single line, and have made a pull request against the oas-resolver repository.

diff --git a/packages/oas-resolver/index.js b/packages/oas-resolver/index.js
index ab2ce37..cb00516 100644
--- a/packages/oas-resolver/index.js
+++ b/packages/oas-resolver/index.js
@@ -466,7 +466,7 @@ function loopReferences(options, res, rej) {
                                 options.openapi = deRef(options.openapi,options.original,{verbose:options.verbose-1});
                                 if (options.verbose>1) console.warn(common.colour.yellow+'Finished internal resolution!',common.colour.normal);
                             }
-                            recurse(options.openapi,{},function(obj,key,state){
+                            recurse(options.openapi,{identityDetection:true},function(obj,key,state){
                                 if (isRef(obj, key)) {
                                     if (!options.preserveMiro) delete obj['x-miro'];
                                 }
lampkin-diet commented 8 months ago

The above commit is necessary to solve another issue. However, I found a simpler solution that just changes a single line, and have made a pull request against the oas-resolver repository.

diff --git a/packages/oas-resolver/index.js b/packages/oas-resolver/index.js
index ab2ce37..cb00516 100644
--- a/packages/oas-resolver/index.js
+++ b/packages/oas-resolver/index.js
@@ -466,7 +466,7 @@ function loopReferences(options, res, rej) {
                                 options.openapi = deRef(options.openapi,options.original,{verbose:options.verbose-1});
                                 if (options.verbose>1) console.warn(common.colour.yellow+'Finished internal resolution!',common.colour.normal);
                             }
-                            recurse(options.openapi,{},function(obj,key,state){
+                            recurse(options.openapi,{identityDetection:true},function(obj,key,state){
                                 if (isRef(obj, key)) {
                                     if (!options.preserveMiro) delete obj['x-miro'];
                                 }

I also checked it locally - works for me. Could we force somehow that fix? And thanks for making such research and fix :)

pauldesmondparker commented 8 months ago

As a temporary measure, you can:

  1. git clone git@github.com:Mermade/oas-kit.git
  2. navigate to the packages/oas-resolver directory
  3. copy the above diff into a file called fix.patch
  4. apply the patch with git apply fix.patch
  5. navigate to veramo repository, then packages/cli directory
  6. pnpm link <relative or absolute path to oas-resolver dir>

You're done.

You could also do:

  1. ...
  2. pnpm link --global # creates a global anchor for oas-resolver to this directory.
  3. navigate to veramo repository, then packages/cli directory
  4. pnpm link --global oas-resolver

But frankly, the first one is less work. You can verify that the node_modules in packages/cli is pointing to the right place with:

  1. ls -al node_modules/oas*

    [!Note] should show a relative path with .../.local/share/pnpm/... (assuming you're using linux).

mirceanis commented 8 months ago

Wow, amazing investigation and kudos for finding a fix. Too bad the fix is not in Veramo directly and we have to wait for an update to oas-resolver.

As you probably already figured, this is not related to the particular plugin method being executed but rather with the parsing of the schema generated by the plugins, and I suspect it might have something in common with #1235

pauldesmondparker commented 8 months ago

@mirceanis Yes, unfortunately we are relying on the maintainers of oas-kit for the resolution of this issue, and there haven't been any PRs merged on that repo since mid 2021.

At present, I see four approaches:

  1. Wait and hope.
  2. decentralized-idenity to fork oas-kit, apply the fix, and use as a sub module.
  3. Pull the relevant code from oas-resolver (small footprint), retaining license notices, and integrate it directly into Veramo.
  4. Find a more active project that offers the same utility. I gave swagger-parser a brief try, but it wasn't a drop in replacement.
mirceanis commented 8 months ago

I just tried downgrading oas-resolver to 1.1.1 and it seems to fix the issue. However, it doesn't work any more for the functionality it was added for in the first place: to help users discover and use the agent API interactively.

Thinking about it a bit more, the execute command of the CLI tool is meant as a shortcut to be able to call any available agent method even when it wasn't programmed as a CLI command. This would be usable for scripting, or calling the local agent from another tech stack, in which case the interactive part doesn't make much sense any more.

I'm leaning toward removing the interactivity from the execute command, along with oas-resolver.

Any thoughts?

pauldesmondparker commented 8 months ago

I guess the question is whether it's bloatware or not.

On one side, there's this:

Stephen King — 'Kill your darlings, kill your darlings, even when it breaks your egocentric little scribbler's heart, kill your darlings.'

On the other hand, this is a plugin driven ecosystem and this feature allows plugin creators to manually investigate the operation of their plugin without the overhead of grokking and implementing the CLI themselves.

FWIW I would vote for retention, after working on the coordinate-mediation v3.

stale[bot] commented 6 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.