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

Implement rent fees. #3813

Merged
merged 2 commits into from
Jul 7, 2023
Merged

Implement rent fees. #3813

merged 2 commits into from
Jul 7, 2023

Conversation

dmkozh
Copy link
Contributor

@dmkozh dmkozh commented Jul 5, 2023

Description

Resolves #3777, #3804 and stellar/rs-soroban-env#903

This also includes cleanup for the expiration bump related code.

Checklist

  • Reviewed the contributing document
  • Rebased on top of master (no merge commits)
  • Ran clang-format v8.0.0 (via make format or the Visual Studio extension)
  • Compiles
  • Ran all tests
  • If change impacts performance, include supporting evidence per the performance document

@dmkozh dmkozh force-pushed the rent_fee branch 3 times, most recently from 12f4b65 to 8c1d1dc Compare July 6, 2023 00:05
continue;
}
bool isTemporary = isTemporaryEntry(lk);
// Enforce minimum expiration for the new entries.
Copy link
Contributor

Choose a reason for hiding this comment

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

The minimum is already set by the host, so you could just set entryChange.newExpirationLedger to the expiration ledger on the entry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we actually need to set it on the host side? I feel like it would be safer if we just had one place where all the validation is performed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well we either have to set it to the min or 0 on creation, and we thought min made more sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually the host logic is currently wrong (off-by-one error, as usual).

I think we should have just one place that handles all the bump logic. If that's host, then it should do all the deduplication, min/max boundary enforcement and autobump logic, such that the final bump vector can be simply applied to the respective entries. Otherwise, all this work should be performed by core. Doing this on the host side is probably nicer for the sake of preflight/CLI etc., so maybe we should move everything there? I would still do that as a followup though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah we can do that as a followup.

This also includes cleanup for the expiration bump related code.
@sisuresh
Copy link
Contributor

sisuresh commented Jul 7, 2023

r+ 07c05a0

@latobarita latobarita merged commit 3fa2a29 into stellar:master Jul 7, 2023
8 checks passed
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.

Charge fees for lifetime extensions
3 participants