-
Notifications
You must be signed in to change notification settings - Fork 529
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
Add token owners to RPC and archive db #10989
Conversation
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.
Could you add a zkapp transaction with non-default tokens to the database used in the replayer test?
@@ -51,6 +51,18 @@ module Public_key = struct | |||
public_key | |||
end | |||
|
|||
module Token_owners = struct | |||
(* hash table of token owners, updated for each block *) | |||
let owner_tbl : Account_id.t Token_id.Table.t = Token_id.Table.create () |
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.
How would this work when the archive process restarts? Should we initialize the table with the tokens from the database?
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.
You're only adding tokens to the db when there's a transaction in a block that mentions them. In that case, those tokens will be in the tokens_used
list, and hence, in this table.
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.
Actually, tokens section in zkapps integration test should be sufficient, right? since it exercises the replayer flow now
We have to add that, right?
let open Deferred.Result.Let_syntax in | ||
let value = Token_id.(to_string token_id) in | ||
match owner with | ||
match Token_owners.find_owner token_id with |
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 finding an owner doesn't mean this is default token, right? (See the comment in Token_owners
module definition)
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.
There should not be any tokens other than the default that have no owner, unless there's a bug, hence the assert
s.
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.
It might be safer to have the table contain options, so that a lookup should never fail.
src/app/archive/archive_lib/diff.ml
Outdated
let token_id = acct.token_id in | ||
let owner = Mina_ledger.Ledger.token_owner ledger token_id in | ||
(token_id, owner) ) | ||
|> List.dedup_and_sort |
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.
Nit: it will usually be more efficient to dedup_and_sort
first, then look up the token_owner
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 accounts_accessed
are all already distinct, but the tokens they use are not.
Just realized what you were trying to say here. I will push a commit with a fix.
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.
Indeed, I meant
- get the tokens from the account IDs
- sort the tokens
- hit the ledger to find out the token owners.
Yes, I can do that, but I think it's for a separate PR. |
a follow up PR? (so that this code change is tested before we make the next qa-net release) |
Actually, tokens section in zkapps integration test should be sufficient, right? since it exercises the replayer flow now |
Add a field
tokens_used
to theBreadcrumb_added
RPC message, a list of token id, owner pairs. Add the same field to precomputed blocks and extensional blocks.The data in that list is used to populate a table of token owners in the archive processor, which is consulted when adding a token id to the archive db.
This change required additional code in
extract_blocks
to find the tokens used in each block. Tested by extracting the blocks from an archive db created with a local testnet, and then runningarchive_blocks
on the resulting blocks.(Note: added an empty token list to the precomputed block sample, in the expectation that these will be regenerated.)
Closes #10772
Closes #10793