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

Bump expiration op #3775

Closed
wants to merge 9 commits into from
Closed

Bump expiration op #3775

wants to merge 9 commits into from

Conversation

sisuresh
Copy link
Contributor

Description

Resolves stellar/rs-soroban-env#759.

Uses xdr from stellar/stellar-xdr#106.

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

@sisuresh sisuresh requested a review from SirTyson June 13, 2023 17:32
Copy link
Contributor

@SirTyson SirTyson left a comment

Choose a reason for hiding this comment

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

We should probably strictly enforce MAX_ENTRY_LIFETIME for manual bumps. For autobumps and contract defined bumps, if the requested lifetime extension goes beyond the maximum, we don't fail, but just bump up to the maximum. Here it might make more sense to actually fail, since the user is manually specifying the bump amount. If a user specifies 10 years of rent in the manual bump op, it's probably best to fail instead of bumping the entry by only a year and silently ignore the 9 years of additional rent.

return false;
}
}
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should set the code to BUMP_EXPIRATION_MALFORMED and fail here if op.ledgersToExpire > networkConfig.maxEntryExpiration to more strictly enforce the max.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

src/transactions/TransactionFrame.cpp Show resolved Hide resolved
// Multiply by 2 for the metadata check since the previous state is
// also included in the meta.
if (resources.extendedMetaDataSizeBytes <
metrics.mLedgerWriteByte * 2 ||
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid integer overflow with multiplication of 2, should we use division instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. This should be changing anyways when we start using EXPIRATION_EXTENSIONS.

if (UINT32_MAX - ledgerSeq > mBumpExpirationOp.ledgersToExpire())
{
bumpTo = ledgerSeq + mBumpExpirationOp.ledgersToExpire();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we cover the else condition as well? Assuming UINT32_MAX is the max Ledger sequence,
bumpTo = Math.min(ledgerSeq + mBumpExpirationOp.ledgersToExpire(), UINT32_MAX)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The initial value of bumpTo is UINT32_MAX, so that else case is already handled.

@sisuresh sisuresh requested a review from dmkozh June 14, 2023 00:40
src/transactions/BumpExpirationOpFrame.cpp Outdated Show resolved Hide resolved
src/transactions/BumpExpirationOpFrame.cpp Outdated Show resolved Hide resolved
@sisuresh sisuresh marked this pull request as ready for review June 20, 2023 23:34
@sisuresh sisuresh force-pushed the bump-op branch 2 times, most recently from a3314d8 to 8a2bc55 Compare June 21, 2023 20:16
@dmkozh
Copy link
Contributor

dmkozh commented Jun 21, 2023

r+ b5386da

latobarita added a commit that referenced this pull request Jun 21, 2023
Bump expiration op

Reviewed-by: dmkozh
@sisuresh sisuresh closed this Jun 22, 2023
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.

Add functionality to pay rent
5 participants