Remove rentEpoch from the JSON RPC API
#7777
Conversation
| Pubkey::from_str(&self.owner).ok()?, | ||
| self.executable, | ||
| self.rent_epoch, | ||
| u64::MAX, |
There was a problem hiding this comment.
This is the point at which I stopped tearing it out. From WritableAccount upward, this line hardcodes rent_epoch to u64::MAX.
There was a problem hiding this comment.
Can you please leave a short comment?
I.e. explaining why this is hard coded?
steveluscher
force-pushed
the
remove-rent-epoch-from-rpc-api
branch
from
aacc343 to
a2fa9ab
Compare
|
If this PR represents a change to the public RPC API:
Thank you for keeping the RPC clients in sync with the server API @steveluscher. |
steveluscher
force-pushed
the
remove-rent-epoch-from-rpc-api
branch
2 times, most recently
from
b0ca9da to
e8ae180
Compare
steveluscher
force-pushed
the
remove-rent-epoch-from-rpc-api
branch
from
e8ae180 to
bf0e468
Compare
|
@brooksprumo, can you route this to whomever is nearest to the ‘rent epoch is now irrelevant’ work? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7777 +/- ##
=======================================
Coverage 83.1% 83.1%
=======================================
Files 812 812
Lines 356900 356858 -42
=======================================
- Hits 296612 296588 -24
+ Misses 60288 60270 -18 🚀 New features to boost your workflow:
|
brooksprumo
left a comment
brooksprumo
left a comment
There was a problem hiding this comment.
Looks good to me. I'm not an expert in the CLI / client-side of RPC, so I'll defer to others on that.
| pub data: UiAccountData, | ||
| pub owner: String, | ||
| pub executable: bool, | ||
| pub rent_epoch: u64, |
There was a problem hiding this comment.
Does this type get returned by any RPC responses? If yes, does that break any parsing/parsers? Mainly wondering for semver reasons.
If not, then 👍 !
There was a problem hiding this comment.
This is the type that gets returned by RPC responses. Any framework that validates on the presence of this field would break.
Removing or modifying the type of a field is generally something you never do, to preserve compatibility with old clients. This PR represents a choice that we have to make:
- Remove this field from the response, knowing that response-validating clients will break.
- Leave this property/value in the response forever.
There was a problem hiding this comment.
@steveluscher if removed, then it will not be returned if someone tries to collect the historical data.
Do we care about that?
There was a problem hiding this comment.
I think all accounts that were subject to rent collection (ie. non rent-exempt) have been reaped and no new ones can be created. I can't think of an RPC query that I could make today that would yield a historical rentEpoch value, but someone in this PR: please double check me.
There was a problem hiding this comment.
Might be relevant to data providers that collect this data like Dune or Allium.
But I'm not sure.
This has always been an going discussion regarding RPC upgrades, if we should remove fields or not.
|
@joncinque - requesting your review to look at the cli/client-side of the RPC stuff. |
|
The downside of leaving this in is wasted space. The downside of removing it is client breakage. We will choose the lesser of these two evils and waste space by leaving it in. One day, when we have #4710 we can safely remove this field in RPC responses. |
4 participants
Problem
SIMD-215 et al have been activated, making
rentEpochirrelevant. anza-xyz/kit#550 removes the type from the JavaScript SDK.Summary of Changes
This PR removes it from the RPC's JSON response.