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

fix: cli: fix panic in lotus-miner actor control list #9241

Merged
merged 3 commits into from Sep 16, 2022

Conversation

rjan90
Copy link
Contributor

@rjan90 rjan90 commented Sep 1, 2022

Tuning down slice to only 6 characters to avoid panics if the multisig-actor is of a really low character length.

Related Issues

fixes #9239

Proposed Changes

Additional Info

lotus-miner actor control list
name    ID      key        use    balance                   
owner   t01044  t01044...         2 FIL                     
worker  t01041  t3wxxc...  other  7.999999626974119012 FIL  

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

Tuning down slice to only 6 characters to avoid panics if the multisig-actor is of really low character length.
@rjan90 rjan90 requested a review from a team as a code owner September 1, 2022 09:56
kstr = kstr[:9] + "..."
kstr = kstr[:6] + "..."
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be a little safer here to check the length k and then print different things based on that. If the string is 9 characters or fewer, print the whole string. If it's 10 or longer, print the first 6 characters with the ....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a check for length of the multisig address, and only printing the first 6 characters with ... if its larger then 9: 3e74819. Else it will print everything.

Check the length of multisig address, and print based on that.
Comment on lines +516 to +518
if len(kstr) > 9 {
kstr = kstr[:6] + "..."
}
Copy link
Contributor

Choose a reason for hiding this comment

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

In an else statement here I would print the whole string (it should fit). And then apply the same thing to lotus-shed/actor.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In an else statement here I would print the whole string (it should fit).

I might be mistaken here, but printing the whole string here would just create an additional output of the key in the wrong place? As kstr is written to the tablewriter and flushed later on.

And then apply the same thing to lotus-shed/actor.go

Oh, completely forgot to fix the lotus-shed cmd 😅

Check the length of multisig address in lotus-shed actor cmd as well.
@magik6k magik6k merged commit e3d5928 into master Sep 16, 2022
@magik6k magik6k deleted the fix/control-list-panic branch September 16, 2022 12:11
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.

actor control list cmd panics when owner is msig
3 participants