CarmineOptions / konoha

A toolkit for DAO-like governance on Starknet
Apache License 2.0
29 stars 43 forks source link

ODHack: Streaming Vesting #79

Closed tensojka closed 2 months ago

tensojka commented 4 months ago

The vesting component should be extended to support streaming of any token.

Definition of Done

The Cairo code works as expected, has nearly full test coverage, is reasonably documented (docstrings or similar suffice) and readable.

See CONTRIBUTING.md

Jemiiah commented 4 months ago

@tensojka pls i would love to work on this

tensojka commented 4 months ago

@Jemiiah Please refer to the Contributor guidelines on what specifically is expected of you when you request to have a complex task assigned.

Jemiiah commented 4 months ago

alright ma'am will check

Jemiiah commented 4 months ago

can I still continue on this work @tensojka

tensojka commented 4 months ago

@Jemiiah Please read the contributor guidelines and provide the required information.

Jemiiah commented 4 months ago

@tensojka I would like to take this task on the 23rd of may and would be concluding by Tuesday that is 6 working days I have also made contributions in the previous OD hack

How do I tackle the issue understanding the requirement Extend the vesting component to support streaming of any token, not just specific ones.

Analyze the Existing Code Review the current vesting component in Cairo and identify parts where token types are handled to generalize them.

Design the Solution Plan modifications to the component for handling any token type, possibly using generalized data structures or interfaces.

Implement the Changes Update the Cairo code to support streaming of any token, ensuring modularity and scalability.

Testing Write and execute comprehensive tests for the new functionality, aiming for nearly full test coverage. Test edge cases and various scenarios to ensure proper functioning.

Documentation Add docstrings or similar documentation to explain changes and functionality. Ensure the code is readable, with clear variable names and necessary comments.

tensojka commented 4 months ago

@Jemiiah Thanks, you have been assigned.

Currently the Vesting component only supports handling and sending the govtoken, note that after your contribution, it should also handle sending any other token.

Make sure to correctly handle storage vars with the streaming and make sure that you can never get more than allotted when claiming multiple times – the math there is a bit tricky as you always need to round down the wei amounts. I would suggest having a storage var with how much has been claimed already in a stream and a storage var for the total streamed amount.

tensojka commented 4 months ago

Thanks @Jemiiah for keeping us updated, I have now unassigned you

BenFaruna commented 4 months ago

Hello @tensojka I would like to work on this.

How I intend to work on this:

I am not entirely sure of how much time this will take, but I will keep you updated as I make progress, if I am assigned to it.

Thank you

tensojka commented 4 months ago

@BenFaruna Can you do the understanding step first and respond with the concrete plans for storage vars? I will then assign you. And I will not assign anyone else for 24 hours from now to give you time to come up with the plans. This is a fairly complex task and will give you an idea of whether you will be able to do the task.

BenFaruna commented 4 months ago

Okay that's great I'll get into the planning now so I can know if I'll be able to undertake it. Thank you

BenFaruna commented 4 months ago

Hello @tensojka, I have looked into the streaming of tokens. It is a fairly complex task, but I believe I can complete it.

Variables:

The stream details will be a struct that holds details about the stream, information like stream ID, the amount deposited, the amount withdrawn, the start time, the deadline, the depositor's address, and the claimer's address.

I will be using tokei as a guide while implementing this feature.

Question

I also noticed some functions of the vesting components are to be called internally but are exposed as external functions, there might be a need to refactor that.

If there is anything I am missing please let me know.

tensojka commented 4 months ago

Why do you need the depositors address? The treasury is always the depositor.

Could we use tokei directly? I didn't know it existed tbh.

It should be implemented as a feature inside the component alongside the existing functions. Maybe if it makes more sense to integrate tokei directly.

What vesting tests do you mean? I don't think there are any so far.

The functions are not exposed internally as this is a component, see comment on line 4.

I don't feel confident in your ability to handle the task and would rather assign it to someone else.

BenFaruna commented 4 months ago

The depositor address can be removed since it Can only be one value.

There are some tests in tests/vesting.cairo. I just tried it out to see how it functions, but they were failing.

Internal functions of components can still be implemented as internal functions and defined as internal implementations on the contract using them. I just mentioned that because I saw it and felt like it should be looked into.

tensojka commented 2 months ago

Done in #112