feat: add /accounts/:id/activities endpoint #906

sborrazas · 2022-09-15T21:23:20Z

Initial approach for dealing with account activities.

This endpoint contains exclusively events generated by direct transactions (no internal transactions generated by contract calls).

The next steps are:

Including contract call events.
Including key-block and micro-block events (e.g. block_mined, dev_reward).
Including AEx9 and AEx141 events.
Refactor AeMdw.Txs so that it uses the Field module as well.

refs #725

Initial approach for dealing with account activities. This endpoint contains exclusively events generated by direct transactions (no internal transactions generated by contract calls). The next steps are: * Including contract call events. * Including key-block and micro-block events (e.g. block_mined, dev_reward). * Including AEx9 and AEx141 events. * Refactor AeMdw.Txs so that it uses the Field module as well.

README.md

test/integration/ae_mdw_web/controllers/activity_controller_test.exs

jyeshe · 2022-09-19T01:37:36Z

README.md

+
+## Activities
+
+Inteded for being able to display all events in which a specific account is related to in any way.


As events are mostly related to logs on blockchain taxonomy, for disambiguation purpose, a description of the concept of event would be helpful.

Maybe action instead of event? Even when the account is passive (like a receiver on SpendTx) there would be an action happening from somewhere.

I think event makes more than action since, since it's related to something that sometimes happens independently of the account given

Would you mind giving some description about the events, that they are not only log events?

Not sure what kind of description you would want, can you send me the description you're referring to so I can include it on the PR?

Any description or examples clarifying that events are not log events would be fine. "For example when an account receives an AE transfer from a SpendTx ..."

README.md

jyeshe · 2022-09-19T02:54:56Z

lib/ae_mdw/fields.ex

+  @typep range() :: {:gen, Range.t()} | nil
+  @typep cursor() :: Txs.tx() | nil
+
+  @create_tx_types ~w(contract_create_tx channel_create_tx oracle_register_tx name_claim_tx ga_attach_tx)a


Following the same logic as :contract_create_tx, the child contracts are indexed likewise with :contract_call_tx and nil pos.

The Field with nil pos the name_claim_tx would be the name hash (not an account). Would it make sense for a use case?

Thank you for clarifying, I removed name_claim_tx from the list, since I think that these (together with contract_create_tx) should be handled differently. The account ID given in the URL does not appear in the tx object being return for these types of events. A new type of event would make more sense (next PR)

:contract_create_tx may be kept for the contract account (would only add the :contract_call_tx for child contracts)

The contract_create_tx is already included. I wanted contracts created through transaction calls to be a separate type of event so we are able to transmit to the user that this isn't a contract call made TO the contract, but to a different contract that creates the account given

Thank you for clarifying, I removed name_claim_tx from the list, since I think that these (together with contract_create_tx) should be handled differently. The account ID given in the URL does not appear in the tx object being return for these types of events. A new type of event would make more sense (next PR)

I was referring to "the account ID given in the URL does not appear, ... A new type of event would make more sense" to say that :contract_create_tx works "for the contract account".

Likewise the :contract_create_tx, the child contract account might appear on the contract call but I also think that it would be better leave it for another kind of event. The only request here is that it should be uniform, if a contract creation will be handled for contract account in a different event, the same would need to be done for child contracts and generalized accounts (or if kept :contract_call_tx would be added).

You're right, I hadn't realized that child contracts were also included in the Field model. I've now removed these to then add them as a separate event later. Thank you!

jyeshe · 2022-09-19T03:00:37Z

lib/ae_mdw/fields.ex

+             {tx_type, tx_field_pos, account_pk, Util.max_256bit_int()}}
+        end
+
+      cursor = if cursor, do: {tx_type, tx_field_pos, account_pk, cursor}


Would rename the latter cursor to make it clear it's a different kind from the one being assigned to.

jyeshe · 2022-09-19T03:20:53Z

test/integration/ae_mdw_web/controllers/activity_controller_test.exs

@@ -0,0 +1,76 @@
+defmodule Integration.AeMdwWeb.ActivityControllerTest do


BlockchainSim was a nice module tool created to easily allow adding new transactions to a blockchain from scratch. It could be used here to be able to have automated unit/controller tests. Even only with :spend_txs, could help validating the activities pagination (if continuation is correct) and if all are returned (later could be used for validating the filters).

Validating all of these future use cases can be done using integration tests, so I would avoid too much mocking that would make the tests lose their value

The reason for the proposal is because there were many data that I needed to test manually. It could be automated somehow to avoid manual regression tests before releases. Besides that we can't have proper integration tests with backward direction. This is what the current tests would be missing for both directions:

if all activities are from the account (on directions)

if the activities pages are complete in a way that the key boundary doesn't exclude some entry (for both directions)

if the next continues from the last of previous page without repetition

if the endpoint renders all fields and if these fields have proper value

I've added some unit tests now

jyeshe

Besides adding the :contract_call_tx for child contracts, some improvement on automated tests is needed (if not on CI with integration ones).

jyeshe · 2022-09-20T00:46:19Z

README.md

+
+## Activities
+
+Inteded for being able to display all events in which a specific account is related to in any way.


Would you mind giving some description about the events, that they are not only log events?

jyeshe · 2022-09-20T01:25:15Z

test/integration/ae_mdw_web/controllers/activity_controller_test.exs

@@ -0,0 +1,76 @@
+defmodule Integration.AeMdwWeb.ActivityControllerTest do


The reason for the proposal is because there were many data that I needed to test manually. It could be automated somehow to avoid manual regression tests before releases. Besides that we can't have proper integration tests with backward direction. This is what the current tests would be missing for both directions:

if all activities are from the account (on directions)

if the activities pages are complete in a way that the key boundary doesn't exclude some entry (for both directions)

if the next continues from the last of previous page without repetition

if the endpoint renders all fields and if these fields have proper value

jyeshe

I would also add unit tests for validating

if the activities pages are complete in a way that the key boundary doesn't exclude some entry (for both directions)
if the next continues from the last of previous page without repetition

but there will be opportunity for that on next PRs

thepiwo

really cool first step! lets tackle AEX-9 next

sborrazas requested review from thepiwo and jyeshe September 15, 2022 21:23

sborrazas self-assigned this Sep 15, 2022

sborrazas added 2 commits September 16, 2022 18:32

fix: return proper structure for activities with same txi

d71e8d5

docs: add activities endpoint to README

c817b27

jyeshe requested changes Sep 19, 2022

View reviewed changes

sborrazas added 2 commits September 19, 2022 17:25

refactor: use plain "type" and "payload" for activities

ebe0806

chore: remove name_claim_tx type from default tx activities

4547c3a

sborrazas requested a review from jyeshe September 19, 2022 20:31

jyeshe requested changes Sep 20, 2022

View reviewed changes

sborrazas added 3 commits September 20, 2022 15:10

test: add activity controller unit tests as well

2767075

docs: add activities/contract events distinction on README

d42a880

chore: remove child contract creations from activities

8730487

sborrazas requested a review from jyeshe September 20, 2022 18:20

jyeshe approved these changes Sep 21, 2022

View reviewed changes

thepiwo approved these changes Sep 21, 2022

View reviewed changes

sborrazas merged commit 950f738 into master Sep 21, 2022

sborrazas deleted the activities branch September 21, 2022 21:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add /accounts/:id/activities endpoint #906

feat: add /accounts/:id/activities endpoint #906

sborrazas commented Sep 15, 2022

jyeshe Sep 19, 2022

jyeshe Sep 19, 2022

sborrazas Sep 19, 2022

jyeshe Sep 20, 2022

sborrazas Sep 20, 2022

jyeshe Sep 20, 2022

sborrazas Sep 20, 2022

jyeshe Sep 19, 2022

sborrazas Sep 19, 2022

jyeshe Sep 20, 2022

sborrazas Sep 20, 2022

jyeshe Sep 20, 2022

sborrazas Sep 20, 2022

jyeshe Sep 19, 2022

jyeshe Sep 19, 2022

sborrazas Sep 19, 2022

jyeshe Sep 20, 2022

sborrazas Sep 20, 2022

jyeshe left a comment

jyeshe Sep 20, 2022

jyeshe Sep 20, 2022

jyeshe left a comment

thepiwo left a comment


		## Activities

		Inteded for being able to display all events in which a specific account is related to in any way.

		@@ -0,0 +1,76 @@
		defmodule Integration.AeMdwWeb.ActivityControllerTest do

feat: add /accounts/:id/activities endpoint #906

feat: add /accounts/:id/activities endpoint #906

Conversation

sborrazas commented Sep 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyeshe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyeshe left a comment

Choose a reason for hiding this comment

thepiwo left a comment

Choose a reason for hiding this comment