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

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

Closed
pauldesmondparker opened this issue Oct 24, 2023 · 12 comments · Fixed by #1347
Closed

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

pauldesmondparker opened this issue Oct 24, 2023 · 12 comments · Fixed by #1347
Labels
bug Something isn't working CLI issues concerning the Veramo command line tool pinned don't close this just for being stale

Comments

@pauldesmondparker
Copy link

pauldesmondparker commented Oct 24, 2023

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:
#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

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:

  • Veramo: 5.4.1
  • Browser: N/A
  • Node Version: v18.12.1
@pauldesmondparker pauldesmondparker added the bug Something isn't working label Oct 24, 2023
@pauldesmondparker
Copy link
Author

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
Copy link

lampkin-diet commented Oct 26, 2023

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
Copy link
Author

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
Copy link
Author

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 added a commit to pauldesmondparker/oas-kit that referenced this issue Oct 27, 2023
caused by commit aabd716 addressing issue Mermade#152
@pauldesmondparker
Copy link
Author

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
Copy link

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
Copy link
Author

pauldesmondparker commented Oct 27, 2023

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:
4. ...
5. pnpm link --global # creates a global anchor for oas-resolver to this directory.
6. navigate to veramo repository, then packages/cli directory
7. 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:
8. ls -al node_modules/oas*

Note

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

@mirceanis
Copy link
Member

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

@mirceanis mirceanis changed the title cli veramo execute -m resolveDid ... fails with Maximum call stack size exceeded cli veramo execute ... fails with Maximum call stack size exceeded Oct 30, 2023
@pauldesmondparker
Copy link
Author

pauldesmondparker commented Oct 31, 2023

@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
Copy link
Member

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
Copy link
Author

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.

mirceanis added a commit that referenced this issue Nov 9, 2023
introducing Debug instead of `console.log` for logging, so that the console can be used for stdout.

fixes #1281
Copy link

stale bot commented Jan 2, 2024

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.

@stale stale bot added the wontfix This will not be worked on label Jan 2, 2024
@mirceanis mirceanis added pinned don't close this just for being stale CLI issues concerning the Veramo command line tool and removed wontfix This will not be worked on labels Jan 2, 2024
mirceanis added a commit that referenced this issue Feb 22, 2024
introducing Debug instead of `console.log` for logging, so that the console can be used for stdout.

fixes #1281
mirceanis added a commit that referenced this issue Feb 22, 2024
using Debug instead of `console.log` for logging, so that the console can be used for stdout.

fixes #1281
nickreynolds pushed a commit that referenced this issue Feb 22, 2024
using Debug instead of `console.log` for logging, so that the console can be used for stdout.

fixes #1281
mirceanis added a commit that referenced this issue Feb 22, 2024
using Debug instead of `console.log` for logging, so that the console can be used for stdout.

fixes #1281
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CLI issues concerning the Veramo command line tool pinned don't close this just for being stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants