-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat: introduce {bi, {txi, local_idx}} for precise internal txs refs #1088
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.
What I like about this PR is that it allows a direct tracking if a name or oracle transition happened due to a specific transaction or after a contract call. I miss only two things:
- This info could be available not only for internal tracking but to the user
- For each mutation changed, would expect the respective test suite to contemplate the new input.
lib/ae_mdw/db/sync/name.ex
Outdated
@@ -251,7 +252,8 @@ defmodule AeMdw.Db.Sync.Name do | |||
m_name_exp = Model.expiration(index: {expire, plain_name}) | |||
m_owner = Model.owner(index: {owner, plain_name}) | |||
m_name_owner_deactivation = Model.owner_deactivation(index: {owner, expire, plain_name}) | |||
%{tx: %{name_fee: name_fee}} = read_raw_tx!(state, txi) | |||
name_claim_tx = DbUtil.read_node_tx(state, txi_idx) |
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.
this is a minor, a refactoring opportunity to use pipes since name_claim_tx
is not used elsewhere
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.
Done
@@ -154,6 +156,33 @@ defmodule AeMdw.Db.Util do | |||
end | |||
end | |||
|
|||
@spec read_node_tx(state(), Txs.txi_idx()) :: Node.tx() | |||
def read_node_tx(state, {txi, -1}) do |
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.
nice refactoring 💯
end | ||
|
||
pointers | ||
def pointers(state, Model.name(updates: [{_block_index, txi_idx} | _rest_updates])) do |
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 we take the opportunity to extend the test cases to cover the internal calls and paying_for
(for ga_meta
would be nice too but we have on name_controller_test
a thorough one)?
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.
Done
lib/ae_mdw/db/sync/contract.ex
Outdated
|> NameRevokeMutation.new(call_txi, block_index) | ||
|> NameRevokeMutation.new({call_txi, local_idx}, block_index) | ||
|
||
{_local_idx, fname, _tx_type, _aetx, _tx, _tx_hash} when fname in @ignored_tx_calls -> |
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 would agree to make a full match to any other event like before in such a way that I would care only about the events that need to be handled (the ignored ones would be the complement set that might change). An pattern match error would be caught by a sync but would save me some time detecting which new one should match.
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.
Pattern-matching error display the structure that it wasn't pattern-matched to. added a raise
anyway
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 was matching any other event before:
ae_mdw/lib/ae_mdw/db/sync/contract.ex
Line 153 in bc54cd6
_int_call -> false |
To ignore internal calls, we would need just like that, for any other that doesn't have to be handled.
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'd rather raise an exception, if we mistakenly add a clause that doesn't match the tx_type
(e.g. {local_idx, "ANES.revoke", :name_foo_tx, ...}
we would never find out about it
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.
fname=AENS.revoke
wouldn't match either for fname in @ignored_tx_calls
but it's okay, as new events are introduced some tests would be able to cover it.
New transactions created by contract calls were only referenced by the txi. With this new implementation, we can now point exactly to the internal contract call that created the transaction, without having to search for it.
NOTE: requies a full re-sync to index all of the existing contract call indexes for claims/updates/transfers/etc