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: add /v2/micro-blocks/:hash/txs endpoint #900

Merged
merged 2 commits into from
Sep 14, 2022
Merged

feat: add /v2/micro-blocks/:hash/txs endpoint #900

merged 2 commits into from
Sep 14, 2022

Conversation

sborrazas
Copy link
Contributor

@sborrazas sborrazas commented Sep 12, 2022

refs #888

@sborrazas sborrazas self-assigned this Sep 12, 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.

Looking neat for me. Some proposals on performance, response structure and testing.

Comment on lines 143 to 144
|> Db.get_micro_blocks()
|> Enum.count()
Copy link
Member

Choose a reason for hiding this comment

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

Same reasoning as #898 (comment)

The proposed change is to add this to AeMdw.Node.Db:

  def micro_block_index(hash), do: micro_block_counter(hash, -1)

  defp micro_block_counter(hash, acc) do
    with block <- :aec_db.get_block(hash),
         :micro <- :aec_blocks.type(block) do
      micro_block_counter(:aec_blocks.prev_hash(block), acc + 1)
    else
      :key -> acc
    end
  end

and use micro_block_index(hash) instead of calling Db.get_micro_blocks() |> Enum.count()

Similar to:

defp micro_block_walker(hash) do
    with block <- :aec_db.get_block(hash),
         :micro <- :aec_blocks.type(block) do
      {block, :aec_blocks.prev_hash(block)}
    else
      :key -> nil
    end
  end

but without accumulating all previous micro blocks and transactions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed in previous PR

Comment on lines +1562 to +1569
"block_hash": "mh_3TzzPsMhgnJBYAtSJ6c4SdbQppZi64mxP61b1u1E8g3stDQwk",
"block_height": 14085,
"hash": "th_2Eo84A8gYkaNnRXkkEe9gPg5jcbKGdPVkZvK9XUSEQhDD6kmqm",
"micro_index": 59,
"micro_time": 1545877257605,
"signatures": [
"sg_E6tbrssPGL4a1mXyN5EW9d3UwRfYN9pSsBDtDEQVyQqTQjhQVBPKNJV6qyc43M5zY2tLE8VQa8Jb3q1XGYKJYaHM5Q3T4"
],
Copy link
Member

Choose a reason for hiding this comment

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

The fields related to the block could be moved to an upper level to avoid repeating them for all transactions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

if this is how the core node exposes the data, then I suggest to keep it as it is

Copy link
Member

Choose a reason for hiding this comment

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

okay, similar to here: https://api-docs.aeternity.io/#/external/GetMicroBlockTransactionsByHash

could you share the reason behind replicating a similar node's structure? it's not 100% the same.

Comment on lines +148 to +149
assert %{"a" => 1} = tx1
assert %{"b" => 2} = tx2
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 have some real fields to validate the rendering at least for the format and values that are not serialized with Node's serialize_for_client?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ended up adding integration tests because I had to much so many node functions that the test became pointless

Base automatically changed from micro-block-v2 to master September 13, 2022 16:18
@sborrazas sborrazas merged commit 2312a8a into master Sep 14, 2022
@sborrazas sborrazas deleted the mb-txs-v2 branch September 14, 2022 09:45
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.

3 participants