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

Multisig actor update #606

Merged
merged 13 commits into from
Aug 6, 2020
Merged

Multisig actor update #606

merged 13 commits into from
Aug 6, 2020

Conversation

austinabell
Copy link
Contributor

Summary of changes
Changes introduced in this pull request:

  • Self explanatory. This one shouldn't be that bad to review

Reference issue to close (if applicable)

Closes #560

Other information and links

vm/actor/src/builtin/multisig/mod.rs Show resolved Hide resolved
vm/actor/src/builtin/multisig/mod.rs Outdated Show resolved Hide resolved
}
}

fn execute_transaction_if_approved<BS, RT>(
Copy link
Contributor

Choose a reason for hiding this comment

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

This fn seems really weird to me, I know its 1:1 matching. It just seems weird that we would be returning an Ok exit code most of the time and wouldn't applied always be true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well the exit code is directly used in a serialized data structure, so I don't see the benefit in not returning the exit code (or how else would you pass that info back, because a result within a result doesn't seem like a good option. (If it's confusing, the ActorError in the return is different from this exit code, and if those three fields weren't directly used I would consider something else like an enum return)

And no applied is only true if the multisig threshold is met and that block of code gets executed

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, so if the multisig threshold is NOT met we still return an Ok exit code even though applied would be false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, they leave as uninitialized, which is the 0 case (ExitCode::Ok) and return that (I think it's a bit weird that they don't make the exit code and return nullable, but I assume this may be more consistent and less likely to panic if things are changed)

@austinabell austinabell merged commit a84ac2f into main Aug 6, 2020
@austinabell austinabell deleted the austin/actup/multisig branch August 6, 2020 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Multisig actor
3 participants