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

scripts: limit the number of stakeholders and managers to 20 #123

Merged
merged 1 commit into from
May 5, 2022

Conversation

rndhouse
Copy link
Contributor

Issue: revault/practical-revault#110

WIP because I haven't ran the revaultd integration tests.

I don't understand why managers are included as stakeholders here (previously hitting stakeholder limit):

DepositDescriptor::new(

@darosior
Copy link
Member

I don't understand why managers are included as stakeholders here (previously hitting stakeholder limit)

That looks like a pretty big oversight. And git blame says it's on me.. I've opened #124 to fix that. Thanks.

Copy link
Member

@darosior darosior left a comment

Choose a reason for hiding this comment

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

Just some comments with regards to the error types, but otherwise looks good to me.
EDIT: ah and it needs a small cargo fmt too :)

src/scripts.rs Outdated Show resolved Hide resolved
src/scripts.rs Show resolved Hide resolved
src/scripts.rs Show resolved Hide resolved
@rndhouse rndhouse force-pushed the limit_stk_mng_count branch 2 times, most recently from 9a0e44f to fb70b2a Compare April 20, 2022 09:29
src/error.rs Outdated Show resolved Hide resolved
@darosior
Copy link
Member

WIP because I haven't ran the revaultd integration tests.

Let me know once that's done. Current state of the PR looks good to me.

@rndhouse rndhouse changed the title WIP: scripts: limit the number of stakeholders and managers to 20 scripts: limit the number of stakeholders and managers to 20 Apr 21, 2022
@rndhouse
Copy link
Contributor Author

I used the this container to run the tests.

Non-server black box tests pass for this branch and revaultd. But note this commit: revault/revaultd@fcc2891

I get this error for server tests on this branch and for master (revaultd):
revault/revaultd#389 (comment)

Copy link
Member

@darosior darosior left a comment

Choose a reason for hiding this comment

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

ACK 02d7443

Thanks!

@darosior darosior merged commit 4bddb1d into revault:master May 5, 2022
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.

2 participants