stellar / rs-soroban-sdk

Rust SDK for Soroban contracts.
Apache License 2.0
118 stars 66 forks source link

Make TTL getters exclude the current ledger. #1269

Closed dmkozh closed 4 months ago

dmkozh commented 4 months ago

What

Make TTL getters exclude the current ledger.

Why

extend_ttl methods always extend the TTL without excluding the current ledger (because the entry has to be alive right now in order to be extensible). However, max_ttl getter currently returns the TTL value that includes the current ledger, thus extending entries by max_ttl is not possible. I've updated max_ttl getter, as well as the newly introduced get_ttl utils to always return TTL without counting the current ledger, so their behavior is more consistent with extend_ttl behavior.

Known limitations

N/A

dmkozh commented 4 months ago

but I'm wondering how we make sure this doesn't break folks who upgrade the SDK and get this change unknowingly

I'm not sure if this can be a problem. Sure, if one had something like assert_eq(env.max_ttl() == 1000) for whatever reason, that would break (but in a very clearly observable fashion). But for any reasonable usage within contracts (i.e. using this inside extend_ttl), this is either a no-op change (for persistent entries), or a bug fix (for temp entries). Moreover, doing extend_ttl(env.max_ttl()) on a temp entry leads to an error both in tests, and on-chain (and that's how we learned about the issue in the first place), so the change will be noticeable as well. FWIW I suspect that we've only noticed this now because there is rarely a reason to arbitrarily extend temp entries to max TTL.