Review design of schedule tracking and aggregation

[raulrpearson]

The getTotalStaked function can be used to get the total VITA staked by a given account, but it'll aggregate balances from all different staking schedules from that user. We can't query the staking duration and staking end in a simple way, which I assume we'd need to implement the perk redemption mechanics that we're going for.

Related to that, there's no way to gather all staking schedules for everyone or for a single staker. Unless Ethereum/Solidity provides automatic getters for contract properties/fields, which I'm not sure it does. More so since the allStakings mapping is declared as private. This could be done with getStakingSchedule by iterating over index values, which are just unsigned integers, but I assume there's a better way.

We could create a subgraph (from the Graph protocol) to index this kind of data from events, for example, but maybe it's possible and overall simpler to have a view function that returns staking schedules for a given account.

Also, does it make sense for an account to have several different staking schedules? Might it be better to implement things in a way where a user could always add more VITA and extend the staking period? That'd be a fundamental design decision that could be made, and it's difficult to surface those with reviews like this. It also falls somewhat on the conceptual/product spec area.

[0xAtum]

We can't query the staking duration and staking end in a simple way, which I assume we'd need to implement the perk redemption mechanics that we're going for.

To keep your contract optimized, I do not store any unrelated data for the SC (Smart Contract). I would suggest your team to use a Subgraph to index the events. If you do not want to use a subgraph, I can add these variables, which will increase the gas cost.

there's no way to gather all staking schedules for everyone or for a single staker.

Two reasons why I'm not supporting it.

1. Main reason is simply, gas optimization. way more cheaper to store data in a mapping variable than an array.
2. Read Gas Limit. Reading a contract uses the same principle as writing, if your reading function exceed the MAX_GAS_LIMIT of the block (30M) it will reverts, making this function nonusable forever if you do reach this limit. I do not know how much CCU you have and how much schedule to expect.

You have two way to handle it.

1. Using multicall and fetch the data -> Uses RPC, -- I put the viem doc, but this can be done with anything.
2. Using a Subgraph and index the events -> Good practice

Might it be better to implement things in a way where a user could always add more VITA and extend the staking period

I implemented the way the scope has been delivered, as so, it was written that a user can have multiple schedules.

Overall, I highly recommend your team to use any subgraph as it gives a better UX to your clients and keep your contracts optimized.

[raulrpearson]

Thanks for the input! I'll have to think about this a bit more.

For now, I don't see a good reason to have multiple locking schedules for a single account. I'll try to get input from the "product people". I think we're much better off having a single locked balance with an associated end timestamp/block number.

We could then have a map from account address to a struct with balance and end fields. We would likely need a way for users to top up their locked balance and period. Maybe we could have a lockTokens function that takes balance and period and it creates a new locking schedule if none exist for the caller. If the caller has an entry in the map, we just accept the tokens, add the new balance and set the end field to whatever the duration parameter requires.

What do you think? Would this simplify things and also/still be gas efficient? Maybe this is not that easy or there's some implications that I'm missing. BTW, just thinking out loud for now, so you can also provide your input if you want, no need to implement these changes just yet.

Of course, this changes things a bit (maybe for the better). I'm not questioning whether you implemented the feature correctly as it was specified to you originally. I'm just trying to understand the vision for the VITA locking feature, for how we want it to materialise, and questioning design choices that maybe are not serving that vision.

[0xAtum]

There are three common approaches to a locking system, each offering different UX and mechanisms, all of which are good / valid:

1. By Snapshots - The current implementation. The most widely used design, straightforward and easy to use.

2. By Average - Define a mathematical formula.

   • More advanced, but can still be simple depending on your approach:
   • Calculate an average of the tokens that would have been unlocked if linear, then return them to the user. This method is not ideal as it undermines the concept of locking.
   • Alternatively, adjust the lock duration based on the average weight of new deposits compared to the current amount. For example, locking 1 unit when already at 1000 should only add 0.01% to the duration. However, locking 10k when there's only 100 should extend the duration by at least 90%.

There are many other mathematical approaches, but your team would need to develop the formula. Unfortunately, I don't have enough time to create one within a reasonable timeframe.

3. By Reset - The approach you are proposing. If someone locks an additional amount, it resets the lock timer.
   • This method is less popular because it either forces users to use another wallet to lock or accept the "penalty" of having their lock time reset.

[raulrpearson]

[By Reset] forces users to [...] accept the "penalty" of having their lock time reset

My suggestion is for the user to be able to create a new locking schedule or to top up the token balance or extend the locking period on an existing schedule at any point. Balance and period would always be parameters. User accounts could claim their tokens if their schedule has elapsed, and admin accounts could terminate any schedule early (maybe even bulk terminate) by setting the schedule end dates to now, which would make them claimable.

One approach would be to hide all the lock creation and expansion logic behind a single lock method, where the user supplies the desired balance and period, and the contract creates that if possible and rejects for a few reasons. Aside from the trivial reasons (amount < minAmount or end < now + minPeriod) we could also reject if the requested schedule would violate the current locking terms (requested balance < current balance or requested end < current end).

With this API, the consumers of the SC would always be able to specify what they want and the contract logic would determine how to make that happen or reject if not possible.

As a specific example, let's say I have an active schedule for 1k VITA, elapsing in 6 months. I could expand my schedule by calling lock(10k VITA, 12 months) and the contract would transfer 9k VITA from my wallet to the smart contract and it would set the end value of the schedule associated with my wallet address to today plus 12 months.

I end up with what I asked for: 10k VITA unlocking in 12 months. This also happens to work if I had no active schedule going on. I ask for lock(10k VITA, 12 months) and I get that same thing.

This works because the final amount and end date requested are bigger than the ones currently in the schedule. If I called lock(3k VITA, 3 months) or lock(500 VITA, 7 months), the call would reject because, in either case, it'd have to relax the existing commitment of having my 1k VITA locked for the next 6 months in order to produce the result I asked for.

A final scenario that comes to mind: let's say I have 10k VITA on an elapsed (claimable) schedule. I now want to have 5k VITA locked for 6 months. Ideally we could just call lock(5k VITA, 6 months) which is what I want, and once more, the contract would try to make that happen: it'd set the terms of the schedule to 5k VITA and end timestamp in 6 months and it'd return the remaining 5k to the calling account (only because the calling account had a starting balance of 10k VITA). The user ends up with what they asked for because the contract could make that happen without breaking any existing commitments.

A tentative interface would be:

• lock(balance, period) already described above.
• claim(balance) method that returns the specified balance to the user if possible, maybe the whole available balance if it's called with no arguments.
• setPeriod(address, period) callable by admin accounts that sets the end field of the schedule associated with that address. Maybe reject if the requested period is bigger than the current one, as this should only be used to terminate schedules earlier, not extend them. setPeriod(address, 0) would make the schedule claimable immediately.
• setPeriod(address[], period) to bulk update schedules, but maybe that's unlikely to ever be necessary and we might just want a master unlock field to avoid having to mutate potentially many schedules when we just for some reason want to let everyone take their tokens out, maybe because we want them to migrate to a new contract or because the "perks for VITA locking" program is cancelled. Maybe setMasterUnlock(bool) so that the master unlock can be set and reset. IDK.
• getSchedule(address) view function that returns the balance and end dates if a schedule exists for that address, and null (or whatever Solidity returns) when no schedule exists. Maybe for convenience also overloaded as getSchedule(address[]) unless that's a bad idea from a gas perspective or whatever (I assume this could be covered with multicall?).

The thinking on this is still ongoing, but interested to hear your thoughts. I'll continue to try to get the rest of the team to chime in, to try to make sure that these mechanics would be aligned with their vision of the feature.

[0xAtum]

Sorry, If I misunderstand, and feel free to correct me if I have something wrong. In short, You wants the current implementation, but with the possibility of updating a current schedule instead always creating a new one ?

If this is correct, the suggested interface will not exactly work; as one address might still have more than one schedule. But, I get the general idea.

I can rewrite the contract so you can do 100% of the fetching on the contract (including the getSchedule), it will add extra gas on execution, but since since EIP-4844 , Ethereum mainnet is better in term of gwei. It's quite recent (2 months ago) and I forgot about it, so the code doesn't need to be as tight

_FYI: an array of integer can have up to 3.5k elements before facing the GASLIMIT of a block in one call.

Let me know on the final verdict from the team.

Have a great weekend

[raulrpearson]

@0xAtum, sorry to have left you waiting for a while. We've had some reorg at VitaDAO recently and things have been in flux. I won't be doing work for VitaDAO any longer, so won't be following up on the issues and feedback. I just read your message on Telegram and I also think your position is very reasonable. Good luck and all the best!

[0xAtum]

@raulrpearson Noted, thank you for letting me know! Wishing you all the best for whatever comes next.