-
Notifications
You must be signed in to change notification settings - Fork 48
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
Getters for stream struct members #31
Conversation
This will be amazing for the lite version of the app or any utility that relies on multicall architecture for data querying (not middleware like the graph). Especially from a DX (dev experience) perspective. One suggestion would be to build a "data aggregation contract" that includes all these extra methods. The core might not need complex interfaces for its logic, so we could just move them in a separate contract as utilities. Might also save us gas on deployments. A thing to keep in mind: we should also measure the gas consumption of these read methods. RPC nodes have a "read-only" gas limit which might break multicall. This is a research task but I wanna find out what this limit is and then figure out how many stream interfaces we can "multicall" at once. |
Great idea!
Yes we should move this to a
Yes, indeed! |
Excellent suggestions from both of you. |
docs: document return variable in natspec for "getAuthorization" docs: improve wording in natspec comments docs: specify the amount units in the natspec comments test: remove "amount" suffix in authorization test trees test: speed up array "assertEq" functions by adding "unchecked"
docs: document "getStream" function in all contracts
I wouldn't merge it to About the code you've written I have nothing to say. Waiting for the special pro functions |
Well they're not just that, they're a holistic part of the repository. E.g. if we implement #29, we should also rename the test contracts for the getters.
You can leave a comment about that in #34. |
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
Adds 7 new getters in the
ISablierV2
interface:getDepositAmount
getRecipient
getReturnableAmount
getSender
getStartTime
getStopTime
isCancelable
These getters make it easier for third-party devs and integrating partners to read data from Sablier.
See related discussion in #25