docs(lockup-contract): refactored README for better clarity #144

telezhnaya · 2021-05-04T15:42:39Z

What have been done:

Added info about the difference between lockup and vesting, and how do we combine these logic
Added info about lockup fields
Added info about foundation account (it could be trird-party)
Removed technical details section. All the info from it is outdated (first point) or written in corresponding sections.

Resolves #140, resolves #141

lockup/README.md

telezhnaya · 2021-05-05T19:57:18Z

I also created additional doc with cute pictures: https://github.com/telezhnaya/docs/blob/master/docs/tokens/lockup_vesting_details.md

frol

Overall, looks accurate to me. Once we have the visualizations, it will be perfect

lockup/README.md

frol · 2021-05-05T22:23:59Z

BTW, I suggest using https://grammarly.com/ to check the grammar 😉

lockup/README.md

maxzaver · 2021-05-10T16:33:11Z

lockup/README.md

+The contract can contain a vesting schedule and serve as a vesting agreement between the Contract Creator and an employee (owner of the contract).
+
+Contract Creator is either the NEAR Foundation, or it could be a third-party account.
+It is set at the moment of initializing the contract by the `foundation_account_id` field (naming could be confusing).


Please explain why it was named like that.

Maybe it's better to delete the information in braces at all?
The explanation is boring. I didn't ask @evgenykuzyakov but I guess, at first, we just didn't think about using other creators apart of NEAR Foundation. Then, after that, the ability to set it was added, but naming was left as it is.
@evgenykuzyakov please correct me if I am wrong.

lockup/README.md

maxzaver · 2021-05-10T16:55:02Z

lockup/README.md


 ### Early Vesting Termination

-In case of vesting schedule, the contract supports the ability for the NEAR Foundation to terminate vesting at any point before it completes.
-If the vesting is terminated before the cliff all tokens are refunded to the Foundation. Otherwise the remaining unvested tokens are refunded.
+In the case of vesting schedule, the contract supports the ability for the Contract Creator to terminate vesting at any point before it completes.


I thought it is foundation_account_id that has control over the termination? Should we define what Contract Creator is?

I thought it is foundation_account_id that has control over the termination?

Yes, that's true.

Should we define what Contract Creator is?

It was defined earlier, in the same paragraph with foundation_account_id.

I believe the confusion here is that "Contract Creator" is ambiguous since on mainnet it is lockup-factory which creates and deploys the contract. My suggestion would be to rename it to "foundation" (without "NEAR" and lowercase) implying any foundation which might be interested in founding lockup contracts and thus become the entity ensuring the trust to terminate vesting early. Our error messages in the contract return messages like:

"Foundation account should be added for vesting schedule"

"Foundation account can't be added without vesting schedule"

"Can only be called by NEAR Foundation"

(the last one is unfortunately too specific)

maxzaver · 2021-05-10T16:56:23Z

lockup/README.md


-In the event of termination, the vesting stops and the remaining unvested tokens are locked until they are withdrawn by the Foundation.
+In the event of termination, the vesting stops, and the remaining unvested tokens are locked until they are withdrawn by the Contract Creator.


This seems like a more detailed explanation of "If the vesting is terminated before the cliff all tokens are refunded to the Contract Creator. Otherwise, the remaining unvested tokens are refunded.", should we make it a bullet list and pre-phase it with "More specifically:"?

I can change it but I try to be not too wordy. I think that my wording gives the same amount of information, doesn't it?

maxzaver · 2021-05-10T16:58:02Z

lockup/README.md

 During termination, the owner can't issue any action towards the staking pool or issue transfers.
-If the amount of tokens on the contract account is less than the remaining unvested balance, the Foundation will try to unstake and withdraw everything from the staking pool.
-Once the tokens are withdrawn from the staking pool, the Foundation will proceed with withdrawing the unvested balance from the contract.
+If the amount of tokens on the contract account is less than the remaining unvested balance, the Contract Creator will try to unstake and withdraw everything from the staking pool.


How can this be possible, if tokens are transferrable from this contract only when they are vested?

It is possible if tokens are on the staking pool

maxzaver · 2021-05-10T17:01:21Z

lockup/README.md


-If there is still a termination balance deficit due to minimum required balance, the owner may decide to fund the deficit on this account to finish the termination process.
-This can be done through a regular transfer action from an account with liquid balance.
+If there is still a termination balance deficit due to the minimum required balance, the owner may decide to fund the deficit on this account to finish the termination process.


This sounds like an important mechanism that everyone should be aware of. Is it possible to pre-fund all lockup contracts upon creation with 35 extra NEAR to avoid having this situation upon termination?

I think we can talk with our financiers and ask them to transfer additional 35 NEAR to the account.

nit: given that we lowered storage costs 10x, 3.5NEAR should be enough

maxzaver · 2021-05-10T17:01:53Z

lockup/README.md

-  - If the NEAR Foundation account ID is provided during initialization, the NEAR Foundation can terminate vesting schedule.
-  - If the NEAR Foundation account ID is not provided, the vesting schedule can't be terminated.
- Lock tokens for the lockup period with the release duration. Tokens are linearly released on transfers are enabled.
+Since the vesting schedule usually starts at the date of employment it allows to de-anonymize the owner of the lockup contract.


To clarify, this contract will be used not just for the employees.

Yes, I know. As I understand, private vesting was used only for Near employees and we don't have plans to use it anymore.

lockup/README.md

frol · 2021-05-12T09:34:58Z

lockup/README.md


 ### Early Vesting Termination

-In case of vesting schedule, the contract supports the ability for the NEAR Foundation to terminate vesting at any point before it completes.
-If the vesting is terminated before the cliff all tokens are refunded to the Foundation. Otherwise the remaining unvested tokens are refunded.
+In the case of vesting schedule, the contract supports the ability for the Contract Creator to terminate vesting at any point before it completes.


I believe the confusion here is that "Contract Creator" is ambiguous since on mainnet it is lockup-factory which creates and deploys the contract. My suggestion would be to rename it to "foundation" (without "NEAR" and lowercase) implying any foundation which might be interested in founding lockup contracts and thus become the entity ensuring the trust to terminate vesting early. Our error messages in the contract return messages like:

"Foundation account should be added for vesting schedule"

"Foundation account can't be added without vesting schedule"

"Can only be called by NEAR Foundation"

(the last one is unfortunately too specific)

telezhnaya · 2021-05-12T17:25:29Z

Some changes were done. I cleaned up the history of commits, I also deleted changes from the code (I fixed some comments there, me and @frol decided not to touch the source code in this PR).

The first commit is the original version + corrections after all reviews. In the second commit, I re-ordered things a little and marked lockup_duration and private vesting as deprecated.

frol

With this change and the change to the high-level overview in the NEAR docs (near/docs#691), I believe, we reached the following:

Confirmed that the implementation itself is correct
The descriptions that were too low-level (reference-like) were extended with more context and references to the high-level ideas
The descriptions that were too generic (leading to uncertainty) were extended with specific examples and illustrations

@telezhnaya Thanks for the thorough review of all the information we had and driving this initiative forward!

maxzaver

Thanks for the documentation fix, approving assuming the remaining comments are resolved.

maxzaver · 2021-05-17T21:17:43Z

lockup/README.md

-The owner can choose the staking pool where to delegate tokens.
-The staking pool contract and the account has to be approved and whitelisted by the foundation, to prevent lockup tokens from being lost, locked or stolen.
+The owner can choose the staking pool for delegating tokens.
+The staking pool contract and the account have to be approved and whitelisted by the NEAR Foundation, to prevent lockup tokens from being lost, locked, or stolen.


It looks so. @evgenykuzyakov please confirm

Waiting for confirmation from @telezhnaya or @evgenykuzyakov

I am not sure but it looks like it's not possible to pass the address of the whitelisting contract

It would be good if we could figure it out from the contract code. If whitelisting contract is hardcoded, then it would be good to have a link to the code where it is defined.

maxzaver · 2021-05-17T21:18:20Z

lockup/README.md

 This staking pool must be an approved account, which is validated by a whitelisting contract.
-Once the staking pool holds tokens, the owner of the staking pool is able to use them to vote on the network governance issues, such as enabling transfers.
-So it's important for the owner to pick the staking pool that fits the best.
+Once the staking pool holds tokens, the owner of the staking pool can use them to vote on the network governance issues, such as enabling transfers.


Is it hardcoded in the contract? Could we please link the place from the source code in the documentation?

frol · 2021-05-19T06:46:00Z

I had to merge ASAP since it is good enough now, and I wanted to remove the scary banner and let partners review the contract without misleading messages. @telezhnaya please, submit a separate PR addressing the concerns from the comments

telezhnaya · 2021-05-19T14:12:55Z

@frol thank you!
I continued here

telezhnaya requested review from frol and evgenykuzyakov May 4, 2021 15:42

telezhnaya requested a review from bowenwang1996 as a code owner May 4, 2021 15:42

bowenwang1996 reviewed May 5, 2021

View reviewed changes

lockup/README.md Outdated Show resolved Hide resolved

lockup/README.md Outdated Show resolved Hide resolved

lockup/README.md Outdated Show resolved Hide resolved

frol reviewed May 5, 2021

View reviewed changes

lockup/README.md Outdated Show resolved Hide resolved

lockup/README.md Outdated Show resolved Hide resolved

lockup/README.md Outdated Show resolved Hide resolved

lockup/README.md Outdated Show resolved Hide resolved

lockup/README.md Outdated Show resolved Hide resolved

evgenykuzyakov reviewed May 6, 2021

View reviewed changes

lockup/README.md Show resolved Hide resolved

lockup/README.md Outdated Show resolved Hide resolved

frol reviewed May 7, 2021

View reviewed changes

lockup/README.md Outdated Show resolved Hide resolved

lockup/README.md Show resolved Hide resolved

maxzaver suggested changes May 10, 2021

View reviewed changes

frol mentioned this pull request May 11, 2021

Possible error in curent version of lockup contract #140

Closed

frol reviewed May 12, 2021

View reviewed changes

telezhnaya added 2 commits May 12, 2021 16:38

#141: Update README in lockup contract

9b265ea

#141: add info about deprecated things, clean up

78426a8

frol approved these changes May 13, 2021

View reviewed changes

frol requested review from maxzaver, evgenykuzyakov and bowenwang1996 May 13, 2021 11:29

telezhnaya mentioned this pull request May 13, 2021

lockup: add vesting info, add graphs with schedule to the doc near/docs#691

Merged

bowenwang1996 approved these changes May 13, 2021

View reviewed changes

telezhnaya changed the title ~~#141: Update README and fix comment in code~~ #141: Update README May 14, 2021

telezhnaya changed the title ~~#141: Update README~~ docs(lockup-contract): refactored README for better clarity May 14, 2021

maxzaver approved these changes May 17, 2021

View reviewed changes

frol merged commit d92f052 into near:master May 19, 2021

telezhnaya mentioned this pull request May 19, 2021

lockup: add info about whitelisting contract to README #147

Merged

telezhnaya self-assigned this May 19, 2021

telezhnaya mentioned this pull request Oct 11, 2021

feat: add NFT mint/transfer/burn event standard near/NEPs#256

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs(lockup-contract): refactored README for better clarity #144

docs(lockup-contract): refactored README for better clarity #144

telezhnaya commented May 4, 2021 •

edited

Loading

telezhnaya commented May 5, 2021

frol left a comment

frol commented May 5, 2021

maxzaver May 10, 2021

telezhnaya May 11, 2021

maxzaver May 10, 2021

telezhnaya May 11, 2021

frol May 12, 2021

maxzaver May 10, 2021

telezhnaya May 11, 2021

maxzaver May 10, 2021

telezhnaya May 11, 2021

maxzaver May 10, 2021

telezhnaya May 11, 2021

frol May 11, 2021

telezhnaya May 12, 2021

maxzaver May 10, 2021

telezhnaya May 11, 2021

frol May 12, 2021

telezhnaya commented May 12, 2021

frol left a comment

maxzaver left a comment

maxzaver May 17, 2021

maxzaver May 17, 2021

frol commented May 19, 2021

telezhnaya commented May 19, 2021


		In the event of termination, the vesting stops and the remaining unvested tokens are locked until they are withdrawn by the Foundation.
		In the event of termination, the vesting stops, and the remaining unvested tokens are locked until they are withdrawn by the Contract Creator.

docs(lockup-contract): refactored README for better clarity #144

docs(lockup-contract): refactored README for better clarity #144

Conversation

telezhnaya commented May 4, 2021 • edited Loading

telezhnaya commented May 5, 2021

frol left a comment

Choose a reason for hiding this comment

frol commented May 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

telezhnaya commented May 12, 2021

frol left a comment

Choose a reason for hiding this comment

maxzaver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frol commented May 19, 2021

telezhnaya commented May 19, 2021

telezhnaya commented May 4, 2021 •

edited

Loading