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

shed: util: get all msig #9322

Merged
merged 4 commits into from
Sep 22, 2022
Merged

shed: util: get all msig #9322

merged 4 commits into from
Sep 22, 2022

Conversation

jennijuju
Copy link
Member

@jennijuju jennijuju commented Sep 15, 2022

image

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

  • All commits have a clear commit message.
  • The PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, INTERFACE BREAKING CHANGE, CONSENSUS BREAKING, build, chore, ci, docs,perf, refactor, revert, style, test
    • area: api, chain, state, vm, data transfer, market, mempool, message, block production, multisig, networking, paychan, proving, sealing, wallet, deps
  • This PR has tests for new functionality or change in behaviour
  • If new user-facing features are introduced, clear usage guidelines and / or documentation updates should be included in https://lotus.filecoin.io or Discussion Tutorials.
  • CI is green

@jennijuju jennijuju requested a review from a team as a code owner September 15, 2022 21:12

ctx := ReqContext(cctx)

ts, err := LoadTipSet(ctx, cctx, api)
Copy link
Contributor

Choose a reason for hiding this comment

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

We can just call the one in cli/ package like we do in other shed commands


var multisigGetAllCmd = &cli.Command{
Name: "all",
Usage: "get all multisig actor on chain with id, siigners, threshold and balance",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Usage: "get all multisig actor on chain with id, siigners, threshold and balance",
Usage: "get all multisig actor on chain with id, signers, threshold and balance",

Comment on lines 106 to 111
Flags: []cli.Flag{
&cli.UintFlag{
Name: "network-version",
Value: uint(build.NewestNetworkVersion),
},
},
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want to pass in the network version. We should be using the network version at whatever tipset we're using otherwise there might be a mismatch. api.StateNetworkVersion(ctx, Tipset key)

Copy link
Contributor

@arajasek arajasek left a comment

Choose a reason for hiding this comment

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

This can be significantly faster (order of seconds) if we change it to work in "offline" mode -- instead of querying a daemon's API, it can work on the daemon's blockstore directly, iterating over all the actors in the state tree. See here for an example shed command.

Copy link
Contributor

@arajasek arajasek left a comment

Choose a reason for hiding this comment

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

Nits but LGTM


var multisigGetAllCmd = &cli.Command{
Name: "all",
Usage: "get all multisig actor on chain with id, siigners, threshold and balance at a tipset",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Usage: "get all multisig actor on chain with id, siigners, threshold and balance at a tipset",
Usage: "get all multisig actor on chain with id, signers, threshold and balance at the given state root",

if builtin.IsMultisigActor(act.Code) {
ms, err := multisig.Load(store, act)
if err != nil {
return err
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make it a practice to always wrap errors? It speeds things up when 🕵️‍♀️ issues.

@arajasek
Copy link
Contributor

And needs make jen

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.

4 participants