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: Add .dryrun Command Support #191

Closed
wants to merge 1 commit into from

Conversation

mayurmarvel
Copy link

@mayurmarvel mayurmarvel commented Mar 14, 2024

Description

This pull request addresses issue #156 by introducing support for the .dryrun command in the aos project. Below is a summary of the changes made:

Files Modified / Created:

  • /src/index.js: Added support for .dryrun command.
  • /src/commands/dryrun.js: Implemented the functionality for the .dryrun command.
  • /src/commands/dryrun.md: Created a usage guide for the .dryrun command.
  • README.md: Updated commands list to include .dryrun.
  • /src/services/help.js: Added .dryrun to the help list.

For the full usage guide and detailed changes, please refer to /src/commands/dryrun.md.

Command Example

  .dryrun ({ Target = 'FgU-RiEaLuC__SHZnI9pSIa_ZI8o-8hUVG9nPJvs92k', Action = 'Balance' })

  .dryrun {Balance}

  .dryrun -r.Messages[0] {Balance}

Flags List:

  • -p: Specifies Process ID.
  • -d: Specifies data.
  • -r: Accesses the Result Object.
  • -v: Views the DryRun Parameters.

Functions:

  • dryrun: Executes a dry run to query Processes without sending a message.

Testing

Performed manual testing to ensure the functionality of the .dryrun command in various scenarios.

Features That Can Be Added

  • A loading spinner can be added if needed.

Related Issue

Closes #156

@mayurmarvel mayurmarvel changed the title feat: Add .dryrun Command Support #156 feat: Add .dryrun Command Support Mar 14, 2024
@twilson63 twilson63 added the hold label Mar 22, 2024
Copy link

@allquantor allquantor left a comment

Choose a reason for hiding this comment

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

Added a couple of comments.
I would re-iterate to reduce overall complexity. A lot of stuff to simplify.
Good job

| `.exit` | This command will exit you console, but you can also do `Ctrl-C` or `Ctrl-D` |

## License

The ao and aos codebases are offered under the BSL 1.1 license for the duration
of the testnet period. After the testnet phase is over, the code will be made
available under either a new
[evolutionary forking](https://arweave.medium.com/arweave-is-an-evolutionary-protocol-e072f5e69eaa)
[evolutionary forking](https://arweave.medium.com/arweave-is-an-evolutionary-protocol-e072f5e69eaa)s

Choose a reason for hiding this comment

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

This s seem to be a typo?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, that's a typo.

await processInput(input, pid, data, owner, id, anchor);
}

async function extractTags(input, pid) {

Choose a reason for hiding this comment

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

This code needs to be simplified. I suggest you reduce complexity by:

  • Removing side effects throwable stuff to the top
  • Generalize the parsing so it cant throw and then put it into a functor
  • Try to reduce conditions to lower complexity

This should give an indication:

async function extractTags(input, pid) {
  // Directly throw an error if input doesn't include "{", avoiding multiple checks.
  if (!input.includes("{")) throw new Error("Tags not specified");

  // Use optional chaining to simplify error handling.
  const content = input.match(/\{(.*?)\}/)?.[1];
  if (!content) throw new Error("Invalid Syntax Usage");

  // Streamline tag parsing and creation with map (the functor), removing redundant else block.
  const tags = content.split(",").map(pair => {
    if (pair.includes("=")) {
      let [key, value] = pair.split("=").map(item => item.trim().replace(/^['"]|['"]$/g, ""));
      // Simplify replacement for "ao.id" with direct approach.
      value = value === "ao.id" ? pid : value;
      return { name: key, value };
    } else {
      // Handle action tags directly within the map function.
      const action = pair.trim().replace(/^'|'$/g, "");
      return { name: "Action", value: action };
    }
  });

  // Check and add "Target" 
  if (!tags.some(tag => tag.name === "Target")) {
    tags.unshift({ name: "Target", value: pid });
  }

  return tags;
}

Choose a reason for hiding this comment

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

P.S if target can be appended you can get rid of else al in all

if (!input.includes("{")) throw new Error("Tags not specified");

  const content = input.match(/\{(.*?)\}/)?.[1];
  if (!content) throw new Error("Invalid Syntax Usage");

  return content.split(",").map(pair => {
    const [key, rawValue] = pair.includes("=") ? pair.split("=") : ["Action", pair];
    const value = rawValue.trim().replace(/^['"](.*)['"]$/, "$1").replace(/^ao\.id$/, pid);
    return { name: key.trim(), value };
  }).concat({ name: "Target", value: pid });

}


const getCustomProcessId = (input, pid) => {

Choose a reason for hiding this comment

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

Slightly cleaner.

const getCustomProcessId = (input, pid) => {
  const match = input.match(/-p=(["'])(.*?)\1/);
  if (!match) return null;
  const customProcessId = match[2] === "ao.id" ? pid : match[2];
  if (customProcessId.length !== 43) throw new Error("Invalid Process Id");
  return customProcessId;
};

const result = await dryrun(dryrunParams);
return result;
} catch (error) {
console.error(chalk.red("Error Performing DryRun:", error.message));

Choose a reason for hiding this comment

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

Would it not be right to propagate the error up in context? Why swallow it here.

@twilson63
Copy link
Collaborator

twilson63 commented Apr 15, 2024

🤔 I think this can be a simpler fix, if you use aos to start a process and you do not have Eval Access, you can open the process in (Read-Only) mode by default, which means, that instead of sending messages, you send dryruns - so you will not be able to perform any write capable functions, because you are not the Owner, but you will be able to inspect the process and visually view the state?

@allquantor @mayurmarvel - What do you think?

For example, if I call aos {pid} and I don't have Eval privileges, then aos does dryrun be default?

@twilson63 twilson63 closed this Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: dryRun support in aos
3 participants