-
Notifications
You must be signed in to change notification settings - Fork 269
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
Introduce StaticAccountData
for ReadOnlyAccount
immutable fields
#2627
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2627 +/- ##
=======================================
Coverage 77.94% 77.94%
=======================================
Files 190 190
Lines 19674 19684 +10
=======================================
+ Hits 15334 15343 +9
- Misses 4340 4341 +1
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this makes sense. Apart from the CI failure of course.
@@ -111,6 +112,7 @@ impl From<UserIdentity> for UserIdentities { | |||
pub struct OwnUserIdentity { | |||
pub(crate) inner: ReadOnlyOwnUserIdentity, | |||
pub(crate) verification_machine: VerificationMachine, | |||
store: Store, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this? The VerificationMachine
already has access to a store type, can't we get things from there?
I guess the point here is that the store inside VerificationMachine
doesn't have a ReadOnlyAccount
anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this? The VerificationMachine already has access to a store type, can't we get things from there?
Yes. No, the "store" in the VerificationStore
contained in the VerificationMachine
was containing the ReadOnlyAccount
before, and now it's only containing a StaticAccountData
instead.
I guess the point here is that the store inside VerificationMachine doesn't have a ReadOnlyAccount anymore?
So yeah, basically that.
382968a
to
cdf9bc1
Compare
Sorry, this commit is a bit big. What happened is that I've pulled a thread: put a `StaticAccountData` there, look at caller; it seems to use only a static account too, so keep up. The only contender was the `OwnUserIdentity` data structure which really wants to sign things. Lucky for us, we could pass it a `ReadOnlyAccount` from a store, in those cases, since we always had a `Store` hanging around; in the future it'll read it from the store cache, which is somewhat identical.
And use the `self.store.account()` when we really need the `ReadOnlyAccount`. Also cache the immutable account data in this Account.
cdf9bc1
to
3398dd1
Compare
This introduces a new data structure,
StaticAccountData
(we could just call itStaticAccount
) that contains all the immutable fields from theReadOnlyAccount
. TheReadOnlyAccount
is "cached" in one place, theStore
; after #2625 lands, we can put theReadOnlyAccount
in the actual cache.Then, all the structs storing
ReadOnlyAccount
are changed according to the following rules:StaticAccountData
ReadOnlyAccount
fields, refer to theStore
'sReadOnlyAccount
The first step was supposed to be carried in small, incremental commits, but I've pulled a thread at some point, and it unraveled lots of uses that could be simplified at once.
This can be reviewed commit by commit. I've left some future work items as
TODO(BNJ)
; can be ideas to try as followups, not guaranteed they'll work fine.Part of #2624.