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

soroban-rpc: Better state expiration support for getLedgerEntry #1010

Closed
dmkozh opened this issue Oct 5, 2023 · 7 comments · Fixed by #916
Closed

soroban-rpc: Better state expiration support for getLedgerEntry #1010

dmkozh opened this issue Oct 5, 2023 · 7 comments · Fixed by #916

Comments

@dmkozh
Copy link
Contributor

dmkozh commented Oct 5, 2023

Currently getLedgerEntry endpoint just returns the LE XDR, which now doesn't have the expiration-related info.

Instead it probably should:

  • Return the entry expiration ledger (fetched from the respective EXPIRATION entry), when applicable
  • Return a flag denoting whether entry has expired, when applicable

This would make the behavior consistent with simulation, where we already do fetch and mark the restored entries (when available).

@dmkozh dmkozh changed the title soroban-rpc: Better state expiration support for getLedgetEntry soroban-rpc: Better state expiration support for getLedgerEntry Oct 5, 2023
@Shaptic Shaptic added this to the Soroban Pubnet Release milestone Oct 5, 2023
@Shaptic
Copy link
Contributor

Shaptic commented Oct 5, 2023

Related, as a more-immediate workaround: stellar/js-soroban-client#153

@tsachiherman
Copy link
Contributor

I don't believe that Return a flag denoting whether entry has expired, when applicable is really needed :
The response include the LatestLedger as well as ExpirationLedger per ledger key.
The client can compare to see if ExpirationLedger < LatestLedger and deduce that the entry have expired.

In the future, when the RPC would want to provide the "information" that the entry has expired, but wouldn't know when, it could return ExpirationLedger == 0, which would align with the existing logic.

@tsachiherman
Copy link
Contributor

@Shaptic @sreuland @dmkozh ☝️

@tsachiherman tsachiherman linked a pull request Oct 11, 2023 that will close this issue
@sreuland
Copy link
Contributor

sreuland commented Oct 11, 2023

I don't believe that Return a flag denoting whether entry has expired, when applicable is really needed

sounds good, so, the net change would reduce to just new 👍

type LedgerEntryResult struct {
	...
        ExpirationLedger int64 `json:"expirationLedgerSeq,string,omitempty"`
}

with additional behavior of:

  1. it's optional, will be absent if not applicable for LE?
  2. if present it could be 0 meaning the LE has no expiration, or non-zero indicates a real ledger number that LE will expire upon.

@tsachiherman
Copy link
Contributor

If it returns, the LE definitely has an expiration. The only question is whether a value of 0 is the right way to say "I don't know which ledger sequence it was expired upon" or whether we should find another way to achieve that.
I think that it's not a decision we have to make right now.

@sreuland
Copy link
Contributor

If it returns, the LE definitely has an expiration. The only question is whether a value of 0 is the right way to say "I don't know which ledger sequence it was expired upon" or whether we should find another way to achieve that. I think that it's not a decision we have to make right now.

ok, is there a provision in spec for any type of LE's to not expire, and therefore have no expiration ledger, where 0 could denote that?

@tsachiherman
Copy link
Contributor

of course. only contract_data and contract_code would expire.. all the rest of the entries have no expiration associated with them ( at least for now ).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants