-
Notifications
You must be signed in to change notification settings - Fork 208
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(inter-protocol): set keyword shares on feeDistributor #6962
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makeShareConfig
should assert that all shares are Nat
.
// Run once immediately for these destinations. | ||
await schedulePayments(); | ||
}, | ||
|
||
/** @param {Record<Keyword, bigint>} newShares */ | ||
setKeywordShares: newShares => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding this to the creatorFacet means that it's accessible only to the Builder DAO, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes; that's the goal of #6926
/** @param {Record<Keyword, bigint>} newShares */ | ||
setKeywordShares: newShares => { | ||
mustMatch(newShares, M.recordOf(M.string(), M.nat())); | ||
keywordShares = newShares; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original keywordShares
is visible in terms
. Is there any visibility for the updated value? It could be on the public facet or pushed to chain storage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can put it on the public facet without significant more work.
Rendering the terms out of date / unreliable does seem awkward, but more challenging to address. Gotta think it over a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that that would be a good goal, but I think we can leave it to address til later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added it to the public facet.
Check for Nat rather than just `bigint` in makeShareConfig(), but also provide `customTermsShape` so that Zoe checks M.nat() before starting the contract.
done. PTAL. |
ICYMI:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGtM
closes: #6926
Description
add
setKeywordShares
creator facet method to fee distributor to facilitate adjustment by BLDer DAO.Security Considerations
using
setKeywordShares()
can render the contract terms outdated, so they are somewhat unreliable.a
getKeywordShares()
method mitigates this somewhat.Scaling Considerations
none?
Documentation Considerations
I suppose there should be some docs to update... I wonder where they are.
Testing Considerations
single happy-path unit test
args have pattern guards, so that seems sufficient