Skip to content
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

Update docs for eager rent collection #10348

Merged
merged 17 commits into from
Jun 10, 2020

Conversation

ryoqun
Copy link
Member

@ryoqun ryoqun commented Jun 1, 2020

Problem

Docs for rent is a bit outdated regarding its timings with the debut of eager rent collection #9527 on mainnet-beta in 1.5 day (epoch 34~).

Ideally, I think the docs update should go first, but the eager rent collection is pursued due to rather urgent security concern and performance degradation due to unbounded live account data growth (#8931).

Summary of Changes

Update the doc for eager rent collection with some bonus clarifications.

Also, note that I intentionally didn't introduce eager-/lazy- rent collection terminology because these are implementation details and named from the historical background. From user's perspective, the eager rent collection looks lazy now, exactly opposite to the naming!

CCs

Any comments are appreciated!

@sakridge: as a reviewer of the actual implementation pr, does this represents the behind intention of the PR well?
@ericlwilliams, @aeyakovenko: regarding recent discussion on economic design for rent, I believe this is exactly what this is implemented in code. Is there any unclear parts the docs still is missing?
@CriesofCarrots: partly related to #10133, originating from #10054, I've added some clarification along with the new mention of eager rent collection. Is there any other omission?

Fixes #10133

@ryoqun ryoqun requested a review from garious June 1, 2020 06:34
@ryoqun
Copy link
Member Author

ryoqun commented Jun 2, 2020

Another FYI!:

@jstarry: Do you have something else to add here, regarding #7413 and various comments you made recently for eager rent collection?

@CriesofCarrots
Copy link
Contributor

@ryoqun , it will be great to get the design docs updated.

I also created a stub page here for a developer-focused doc on rent and accounts: https://docs.solana.com/apps/rent
I was hoping we could offer a straightforward, functional introduction to rent here. If you want to take an initial crack at it, that would be great. But if not, let's keep #10133 open.
CC @danpaul000, I think that would fall under your current focus?

@mvines mvines removed the v1.1 label Jun 3, 2020
@ryoqun ryoqun mentioned this pull request Jun 3, 2020
5 tasks
@ryoqun
Copy link
Member Author

ryoqun commented Jun 3, 2020

I was hoping we could offer a straightforward, functional introduction to rent here. If you want to take an initial crack at it, that would be great.

Okay, I'll take the initial crack. :)

@ryoqun ryoqun force-pushed the eager-rent-collection-docs branch from 2ef4cdb to e9eea03 Compare June 5, 2020 13:14
@garious garious requested a review from ericlwilliams June 5, 2020 13:23
Copy link
Contributor

@ericlwilliams ericlwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of comments / questions on the write-up. Thanks!

docs/src/apps/rent.md Outdated Show resolved Hide resolved
docs/src/apps/rent.md Outdated Show resolved Hide resolved
docs/src/apps/rent.md Outdated Show resolved Hide resolved
docs/src/apps/rent.md Outdated Show resolved Hide resolved
@ryoqun
Copy link
Member Author

ryoqun commented Jun 5, 2020

@ericlwilliams Thanks for reviewing! I've addressed your comments and clarified the text a bit. :)

docs/src/apps/README.md Outdated Show resolved Hide resolved
docs/src/apps/rent.md Outdated Show resolved Hide resolved
docs/src/apps/rent.md Outdated Show resolved Hide resolved
docs/src/apps/rent.md Outdated Show resolved Hide resolved
docs/src/apps/rent.md Outdated Show resolved Hide resolved
docs/src/apps/rent.md Outdated Show resolved Hide resolved
docs/src/apps/rent.md Outdated Show resolved Hide resolved
docs/src/apps/rent.md Outdated Show resolved Hide resolved
docs/src/apps/rent.md Outdated Show resolved Hide resolved
docs/src/apps/rent.md Outdated Show resolved Hide resolved
Thank you so much!

Co-authored-by: Tyera Eulberg <[email protected]>
docs/src/apps/rent.md Outdated Show resolved Hide resolved
@ryoqun ryoqun requested a review from CriesofCarrots June 8, 2020 04:31
@ryoqun
Copy link
Member Author

ryoqun commented Jun 8, 2020

@CriesofCarrots I think I've addressed all your comments. :)

docs/src/apps/README.md Outdated Show resolved Hide resolved
docs/src/apps/rent.md Outdated Show resolved Hide resolved
docs/src/apps/rent.md Outdated Show resolved Hide resolved
docs/src/apps/README.md Outdated Show resolved Hide resolved
docs/src/apps/README.md Outdated Show resolved Hide resolved
docs/src/apps/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@garious garious left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to block on my review. I haven't had a chance to review in depth, but looks quite detailed! Thanks for writing this up.

@ryoqun
Copy link
Member Author

ryoqun commented Jun 9, 2020

No need to block on my review. I haven't had a chance to review in depth, but looks quite detailed! Thanks for writing this up.

@garious I see!

From @CriesofCarrots:

I also created a stub page here for a developer-focused doc on rent and accounts: https://docs.solana.com/apps/rent
I was hoping we could offer a straightforward, functional introduction to rent here. If you want to take an initial crack at it, that would be great. But if not, let's keep #10133 open.
CC @danpaul000, I think that would fall under your current focus?

@danpaul000 Hi! Do you mind having a look at this pr? If there is no objections, I want to merge this because I think it's getting mature to merge. :)

@danpaul000
Copy link
Contributor

No need to block on my review. I haven't had a chance to review in depth, but looks quite detailed! Thanks for writing this up.

@garious I see!

From @CriesofCarrots:

I also created a stub page here for a developer-focused doc on rent and accounts: https://docs.solana.com/apps/rent
I was hoping we could offer a straightforward, functional introduction to rent here. If you want to take an initial crack at it, that would be great. But if not, let's keep #10133 open.
CC @danpaul000, I think that would fall under your current focus?

@danpaul000 Hi! Do you mind having a look at this pr? If there is no objections, I want to merge this because I think it's getting mature to merge. :)

Looks good to me @ryoqun , thanks for the write-up!

@ryoqun ryoqun merged commit 40ffc56 into solana-labs:master Jun 10, 2020
mergify bot pushed a commit that referenced this pull request Jun 10, 2020
* Update docs for eager rent collection

* Add rent doc and clarify account doc for app devs

* Clarify some and pass the grammarly

* Fix units notation

* Fix link

* Fix link really

* Fix link really really

* More grammarly

* Apply suggestions from code review

Thank you so much!

Co-authored-by: Tyera Eulberg <[email protected]>

* Add explanation of 19.055441478439427

* Fix unit...

* Fix unit...

* Clarify rent duration reasoning

* Tweak a text for more clarification

* Tweak more..

* Apply suggestions from code review

Co-authored-by: Tyera Eulberg <[email protected]>

* Revert too detailed out-of-context explanations

Co-authored-by: Tyera Eulberg <[email protected]>
(cherry picked from commit 40ffc56)
solana-grimes pushed a commit that referenced this pull request Jun 10, 2020
danpaul000 pushed a commit to danpaul000/solana that referenced this pull request Jul 8, 2020
* Update docs for eager rent collection

* Add rent doc and clarify account doc for app devs

* Clarify some and pass the grammarly

* Fix units notation

* Fix link

* Fix link really

* Fix link really really

* More grammarly

* Apply suggestions from code review

Thank you so much!

Co-authored-by: Tyera Eulberg <[email protected]>

* Add explanation of 19.055441478439427

* Fix unit...

* Fix unit...

* Clarify rent duration reasoning

* Tweak a text for more clarification

* Tweak more..

* Apply suggestions from code review

Co-authored-by: Tyera Eulberg <[email protected]>

* Revert too detailed out-of-context explanations

Co-authored-by: Tyera Eulberg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rent implementation is undocumented
8 participants