Skip to content
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

Merged
merged 3 commits into from Dec 23, 2022

Conversation

sborrazas
Copy link
Contributor

@sborrazas sborrazas commented Dec 20, 2022

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

@sborrazas sborrazas self-assigned this Dec 20, 2022
Copy link
Member

@jyeshe jyeshe left a 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.

@@ -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)
Copy link
Member

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

Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

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)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

|> 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 ->
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

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:

_int_call -> false

To ignore internal calls, we would need just like that, for any other that doesn't have to be handled.

Copy link
Contributor Author

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

Copy link
Member

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.

@sborrazas sborrazas changed the title feat: introduce {bi, {txi, local_idx}} structure for precise… feat: introduce {bi, {txi, local_idx}} for precise internal txs refs Dec 20, 2022
@sborrazas sborrazas merged commit e5df7b5 into master Dec 23, 2022
@sborrazas sborrazas deleted the bi-txi-idx branch December 23, 2022 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants