Leasable - Time-based lifecycle contract


#1

Hi everyone, I just contributed this last night. I would like to get some feedback. The test shows an example implementation.

My goal is to create a high level time-based life cycle contract that can be used for any scenario. This can be used in places like TimedCrowdsale and TokenTimelock.

Some questions:

  1. Should I add more modifiers to allow for any time scenario. For example onlyBeforeLeaseStart is not added.
  2. Will code coverage be effected if I am testing Leasable indirectly in LeasableMock? If so, how can I fix this?
  3. Any other suggestions for improvement?

#2

Does anyone know why this is failing? Leasable.test.js passes locally


#3

Hi Sam! Glad to have you here. We loved the polish of your PR!

I think a feature like this makes sense given that the concept of time window shows up in several places in OpenZeppelin. I think this concept should be encoded in a library with a struct and functions to check if the time window is ongoing, etc. What do you think?

As for the functionality that you proposed in Leasable, I see it as providing everything needed for tying the lifecycle of a contract to one time window. So as you said this applies to TimedCrowdsale, TokenTimelock, also probably TokenVesting. I think having this contract also makes sense. Can you share the specific example where you personally found the need for this functionality?

I don’t think Leasable is a good name though. There is no actual lease going on, right?


#4

Yes, I don’t really see a reason not to consider all cases with these.

The way the coverage tool works is by inserting events in between statements, so it’ll check whether a statement was reached, independently of how it was reached (so using LeasableMock will work). I find it’s better to write tests attempting to cover all cases, and then check with the tool to see if I’ve missed any, instead of having it tell me what’s missing from the get go: there are some conditions it is not able to detect.

I’m not sure - it’s weird because the unit test run passes fine, but the coverage run fails. We’ve seen some weird behaviour when using ganache and time functions (related to snapshots, iirc) that goes away when having it mine mine an initial block in a before block: see here for an example.

We also may want to have a measure of how much time has elapsed, while inside the window (maybe in seconds? days?), to e.g. allow for IncrasingPriceCrowdsale, TokenVesting, etc.


#6

I agree that a library would be better here. Good call. I’ll add it!

I initially used it in an escrow / payroll Dapp that tracked time worked for each employee. Here is a snippet of one of the methods (needs some refactoring…). This governs when employees and subcontractors are allowed to submit their time. The billable time window is “Leased” out. One of the methods (not shown below) has a notBeforeLeaseEndFor(client) modifier which I found very useful. It was used to prevent a client from triggering a pro-rated refund before the end date.

        public
        boundByLease
    {
        Invoice storage invoice = invoices[_invoiceIndex];
        require(invoice.worker == msg.sender, "You are not authorized.");

        uint newTotalTimeWorked = invoice.totalTimeWorked.add(_time);
        require(
            newTotalTimeWorked <= invoice.maxBillableTime,
            "You cannot exceed the maximum allowed time."
        );
        invoice.totalTimeWorked = newTotalTimeWorked;

        uint totalBillForTimeAdded = invoice.billableRate.mul(_time);
        uint workerWage = invoice.workerRate.mul(_time);
        uint ownerWage = totalBillForTimeAdded.sub(workerWage);

        budget.pendingWageWithdrawalTotal = budget
            .pendingWageWithdrawalTotal
            .add(totalBillForTimeAdded);
        budget.reservedInvoiceBudget = budget.reservedInvoiceBudget.sub(totalBillForTimeAdded);

        pendingWithdrawals[invoice.worker] = pendingWithdrawals[invoice.worker].add(ownerWage);
        pendingWithdrawals[owner] = pendingWithdrawals[owner].add(ownerWage);

        emit TimeSubmitted(_invoiceIndex, _time, address(this));
    }

The term “Lease” is often used for the expiration date of a cookie in a browser. In my mind “Leasable” works in this sense. You are giving rights out for a specified period of time. I am open to another term if there are suggestions.

Thanks for the feedback. Good stuff~


#7

I love the idea of returning elapsed time. I’ll add something like that to the library.

Thanks for the links. I’ll look into the tests when I have some time.


#8

Where there multiple start and end times then? How was that handled?


#9

No, there was one start/end for the whole contract, then I had multiple instances of a struct called “Invoice” which contained the worker address, rate, invoicable hours (seconds as uint256), and max billable time. No worker for an invoice could track time outside of the “Lease”.


#10

@Sam updated his PR, renaming the contract to TimeFrame: https://github.com/OpenZeppelin/openzeppelin-solidity/blob/bb5de75c7a60e816128f6672556ac5a54e4bc34f/contracts/lifecycle/TimeFrame.sol

The newly added functions (e.g. elapsedSinceStart, length, etc.) are quite nice, I think they’ll be very helpful when using the lib (e.g. contracts may require lengths to not be shorter than a certain amount, etc.). I was somewhat surprosed by terminate, but I guess its existence does make sense in multiple scenarios.

@frangio WDYT? The trend towards libraries (Roles, Counter) is a good thing IMO, abstracting away complexity into simple primitives. Is there anything else you’d like to see here, or something you’re not sure about?


#11

Nice! Yes, I love libraries like this. I also want us to push for closed structs in Solidity, which will make this kind of thing better.

I’m not sure I understand the purpose of the createdAt field. @sam would you mind explaining that a bit?

Also, since the struct members have invariants/preconditions, I think we should provide a function to create struct instances.


#12

I plus-oned the closed struct issue on github. That would rock.

The createdAt field is part of a contract’s time line so I added it. My first response is I’m not sure how everyone would use it, but it falls under the category and seemed appropriate. I like being abstract about things instead of designing a library exclusively for, let’s say, an Escrow. One situation where created at might be useful is in a statement of work agreement between a consultancy and a client. Many SOWs have a clause that states “This muse be executed by both parties within 10 days or it is rendered null.” The start and end dates are both specified some time in the future, but the createdAt date is usually the time when a consultancy signs their end with a date.

createdAt can be used in so many ways, but I wasn’t concerned with how people would use it per se. It feels like something that should be part of a TimeFrame.

With that said, I removed it :smile:


#13

We totally share this mindset, but we also try not to add things before we need them because we think we can have a better design once it’s guided by actual needs. So we try to design incrementally.

I’ll take a look at the PR soon!