-
Notifications
You must be signed in to change notification settings - Fork 80
feat: NTX Actor support for notes that predate account creation #1324
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
base: ntx-refactor
Are you sure you want to change the base?
Conversation
crates/ntx-builder/src/builder.rs
Outdated
| // Cache notes that predate corresponding accounts if there are any. | ||
| if let Ok(prefix) = NetworkAccountPrefix::try_from(account_delta.id()) { | ||
| self.coordinator.cache_predating_events(prefix, &event, id); | ||
| } |
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.
@bobbinth this is probably not sufficient. I suppose I need to search the prefixes involved within every note of the transaction? Not just the one pertaining to the account delta?
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.
Yes, in fact, I don't think the account from account delta is relevant in this context. Basically, for every transaction we need to get a list of output notes (we only care about network notes addressed to a single account), and then route them to their respective accounts. The notes that remain "unassigned" after this process should be treated separately (i.e., put into some pool from which they can be assigned to an account if/when it gets created).
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.
The model we have right now is that we broadcast every event to the actors. Initially we did instead filter out irrelevant actors but we removed that logic for some reason (lost in sea of resolved comments in first PR).
This commit shows the logic we removed. Essentially:
fn find_affected_accounts(
account_delta: Option<&AccountUpdateDetails>,
network_notes: &[NetworkNote],
) -> HashSet<NetworkAccountPrefix> {We might want to go back to this both for this PR and in the future if we decide to only spawn ephemeral tasks rather than keeping a task around for every actor.
0c15bba to
649b73b
Compare
Mirko-von-Leipzig
left a comment
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.
Not a full review, but one issue I did pick up which needs thinking about
Mirko-von-Leipzig
left a comment
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 this works.
I would like us to work on simplifying the overall flow somehow. I've been thinking about what assumptions we can make to do this and I have an idea I'll write up soon.
Context
There is an edge case whereby transactions that relate to not yet existent network accounts are never processed by the NTB. See #1317.
Closes #1317.
No changelog as this is being merged into tracking branch.
Changes
Arcto reduce cost of cloning.