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

Remove orchestrator.makeLocalAccount which is basically a shortcut for orchestrator.getChain("agoric").makeAccount() #10106

Closed
ivanlei opened this issue Sep 18, 2024 · 2 comments · Fixed by #10260
Assignees
Labels
enhancement New feature or request

Comments

@ivanlei
Copy link
Contributor

ivanlei commented Sep 18, 2024

What is the Problem Being Solved?

orchestrator.makeLocalAccount is basically a shortcut for orchestrator.getChain("agoric").makeAccount(). While potentially helpful, this helper/shortcut can confuse a developer by creating multiple ways to accomplish the same task.

Description of the Design

I intend to remove the public facet method and use compilation/testing to verify that all vestiges, callers, and related examples are gone.

Security Considerations

None. This is removing code that doesn't uphold any interesting security property. All the same functionality is still available to contract devs - just through a slightly modified bit of code.

Scaling Considerations

None.

Test Plan

Red code. If all tests continue to pass we're solid.

Upgrade Considerations

None. This isn't upgrading/changing something live on chain.

@dckc
Copy link
Member

dckc commented Sep 18, 2024

We seem to have API reference docs that include this method:
https://docs.agoric.com/guides/orchestration/getting-started/api.html#makelocalaccount

So please add a PR or issue or whatever to remove it there too.
Maybe just get rid of that whole page in favor of the @agoric/orchestration package reference docs?

@turadg
Copy link
Member

turadg commented Oct 11, 2024

makeLocalAccount returns a LocalChainAccount whereas getChain('agoric').makeAccount() returns an OrchestrationAccount that is parameterized with Agoric chain capabilities.

Also, the latter is to be used in place of the former, the jsdoc needs updating because it says "remote chain":

  /**
   * Creates a new account on the remote chain.
   * @returns an object that controls a new remote account on Chain
   */
  makeAccount: () => Promise<OrchestrationAccount<CI>>;

@0xpatrickdev 0xpatrickdev assigned 0xpatrickdev and unassigned ivanlei Oct 11, 2024
0xpatrickdev added a commit that referenced this issue Oct 11, 2024
0xpatrickdev added a commit that referenced this issue Oct 11, 2024
0xpatrickdev added a commit that referenced this issue Oct 11, 2024
0xpatrickdev added a commit that referenced this issue Oct 11, 2024
0xpatrickdev added a commit that referenced this issue Oct 15, 2024
@mergify mergify bot closed this as completed in #10260 Oct 16, 2024
@mergify mergify bot closed this as completed in 5526337 Oct 16, 2024
mergify bot added a commit that referenced this issue Oct 16, 2024
closes: #10106

## Description
There doesn't seem to be a motivating use case for exposing a maker for a low-level `LocalChainAccount` in the `Orchestrator`, so lets remove it to avoid misdirection. Instead, consumers can call `orch.getChain('agoric').then(c => c.makeAccount())` to get a `LocalOrchestrationAccount`.

### Security Considerations
n/a

### Scaling Considerations
n/a

### Documentation Considerations
docs.agoric.com already reflects this: Agoric/documentation#1208 (comment)

### Testing Considerations
Removing code, so existing tests suffice

### Upgrade Considerations
Library code that will go out in a NPM Orch Release
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants