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

feat(run-protocol): collect runStake fees #5197

Merged
merged 2 commits into from
Apr 22, 2022
Merged

Conversation

turadg
Copy link
Member

@turadg turadg commented Apr 22, 2022

Description

  • add makeCollectFeesInvitation to RunStake's creator
  • fix types
  • add runStakeCreator to the list in startRewardDistributor

Security Considerations

RunStake creator facet exposes a way to collect its fees

fixes: #5004

Documentation Considerations

--

Testing Considerations

Relying on types. Open to requests for additional coverage.

@turadg turadg force-pushed the ta/runStake-collectFees branch from 4d425ad to c9f8eef Compare April 22, 2022 18:35
Copy link
Contributor

@Chris-Hibbert Chris-Hibbert left a comment

Choose a reason for hiding this comment

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

LGTM

packages/run-protocol/src/runStake/runStake.js Outdated Show resolved Hide resolved
Co-authored-by: Chris Hibbert <[email protected]>
@turadg turadg added the automerge:squash Automatically squash merge label Apr 22, 2022
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

I defer to @Chris-Hibbert ; I'm fuzzy on makeGovernorFacet(creatorFacet)

Comment on lines -207 to +214
return { publicFacet, creatorFacet };
return { publicFacet, creatorFacet: makeGovernorFacet(creatorFacet) };
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean runStake governance was previously busted? Did we not have any tests for it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just moved the makeGovernorFacet down here to make more obvious that there are two slightly different things named creatorFacet.

@mergify mergify bot merged commit ac1b9c8 into master Apr 22, 2022
@mergify mergify bot deleted the ta/runStake-collectFees branch April 22, 2022 19:13
@dckc
Copy link
Member

dckc commented Apr 22, 2022

Darn; I think the permit for that needs tweaking:

modified   packages/run-protocol/src/core-proposal.js
@@ -137,6 +137,7 @@ const REWARD_MANIFEST = harden({
       loadVat: true,
       vaultFactoryCreator: true,
       ammCreatorFacet: true,
+      // TODO: runStake
       zoe: true,
     },
     produce: {

test-boot.js used to verify all the permits, but I think it only tests the platform bootstrap ones, not the run-protocol ones, now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RUNstake fees are wired up correctly
3 participants