Extract account-info crate #2429
Conversation
kevinheavey
force-pushed
the
extract-account-info
branch
2 times, most recently
from
d84ff8a to
a13d538
Compare
|
The Firedancer team maintains a line-for-line reimplementation of the |
kevinheavey
force-pushed
the
extract-account-info
branch
2 times, most recently
from
cba6424 to
82af211
Compare
|
@yihau since this is very close, can you accept ownership of |
kevinheavey
force-pushed
the
extract-account-info
branch
from
91885ce to
773906e
Compare
kevinheavey
added
the
automerge
|
automerge label removed due to a CI failure |
mergify
bot
removed
the
automerge
kevinheavey
force-pushed
the
extract-account-info
branch
from
27d8f46 to
1f9b33b
Compare
kevinheavey
added
the
automerge
| @@ -28,7 +29,7 @@ pub struct AccountInfo<'a> { | |||
| /// Program that owns this account | |||
| pub owner: &'a Pubkey, | |||
| /// The epoch at which this account will next owe rent | |||
| pub rent_epoch: Epoch, | |||
| pub rent_epoch: u64, | |||
There was a problem hiding this comment.
Why was the Epoch type replaced with u64? Since we're using Pubkey, can we keep using Epoch too, please?
There was a problem hiding this comment.
Pubkey is a struct, and not a type, so it would be a breaking change to use anything else.
I might be missing something, but what's the reasoning to prefer the Epoch type alias? The idea was to avoid an extra dependency.
There was a problem hiding this comment.
Pubkey is a struct, and not a type, so it would be a breaking change to use anything else.
A struct is a type ;)
I'd prefer if Epoch were a proper type too, and not just an alias. (But that's orthogonal to this discussion.)
The idea was to avoid an extra dependency.
Ah, that part makes sense. How "expensive" is it to have this extra dependency?
what's the reasoning to prefer the Epoch type alias?
Strong typing is beneficial for preventing errors/making refactors easier/etc. Since Epoch is a type alias, we lose some of that, but we still retain search and the self-documenting aspects of Epoch.
There was a problem hiding this comment.
Ah, that part makes sense. How "expensive" is it to have this extra dependency?
Maybe @kevinheavey has some numbers on that, or I can whip them up
Since Epoch is a type alias, we lose some of that, but we still retain search and the self-documenting aspects of Epoch.
Gotcha, I can see from the documenting aspect that it could be useful. My thinking was that:
- I've never seen the rent epoch used by a program
- it introduces another dependency
- since it's just a type alias, it doesn't really matter
For those reasons, I lean towards keeping it as is, but if you feel strongly about it, we can change it.
There was a problem hiding this comment.
These points make sense to me, especially since this type is meant to be program-facing. The internal-facing structs for accounts can/do keep using Epoch, so I don't think any changes are necessary. Thanks for taking the time to explain this to me!
There was a problem hiding this comment.
Sounds good! And thanks for clarifying the difference between type and type alias 😅
There was a problem hiding this comment.
yeah on my machine adding the solana-clock dependency increases solana-account-info build time by 40%. It's made quite expensive by the proc macro deps in solana-clock
* extract account-info crate * make bincode optional in account-info crate * update lock file * update doc examples * remove solana-program from doc examples * remove solana-program from dev deps * fmt * move MAX_PERMITTED_DATA_INCREASE to account-info crate
Problem
account_info.rs needs to be made available outside solana-program
This branches off #2413 so that needs to be merged first
Summary of Changes