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

Add getGenesisAddress query to graphql #808

Merged
merged 15 commits into from
Jan 13, 2023
Merged

Add getGenesisAddress query to graphql #808

merged 15 commits into from
Jan 13, 2023

Conversation

herissondev
Copy link
Contributor

Description

This Pr aims to add a getGenesisAddress query to graphql to make it easier to find the genesis address of a transaction.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Start a node, open the explorer, find a transaction and query the graphql endpoint to retrieve the genesis address.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@samuelmanzanera samuelmanzanera added the external contribution Contribution by non core team label Jan 5, 2023
@@ -18,6 +18,30 @@ defmodule ArchethicWeb.GraphQLSchema.Resolver do

@limit_page 10

def get_genesis_address(address) do
t1 = Task.async(fn -> TransactionChain.fetch_genesis_address_remotely(address) end)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hello, ArchethicWeb should be agnostic to the underlying architecture of the archethic project ( it should only interact with the public api in the Archethic module).
I would suggest to change this call to Archethic.fetch_genesis_address_remotely

Copy link
Contributor

@tenmoves tenmoves Jan 5, 2023

Choose a reason for hiding this comment

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

you don't need a Task here, I would call directly the function in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello, ArchethicWeb should be agnostic to the underlying architecture of the archethic project ( it should only interact with the public API in the Archethic module).

Ok thanks, I'll fix that! Could you explain to me why it should be agnostic to the underlying architecture of the archethic project, I don't really understand?

Copy link
Contributor

Choose a reason for hiding this comment

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

ArchethicWeb represents the frontend and Archethic represents the backend logic, in here they are in the same repository but this can change someday in the future, also we try to follow the hexagonal architecture (some call it clean architecture).

Finally if the web only depends on the public api, it means that if we refactor the whole backend tomorrow we will not need to change the web part and separate people / teams can work independently on different parts of the project.

Feel free to ping me if it isn't clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very clear, thanks for the explanation!

I tried using the call Archethic.fetch_genesis_address_remotely as you suggested however it seems to always return the same address as entered.

here is the code:

def get_genesis_address(address) do
    case Archethic.fetch_genesis_address_remotely(address) do
      {:ok, genesis_address} ->
        {:ok, %{genesis: genesis_address}}

      _ ->
        {:ok, %{genesis: address}}
    end
end

Correcting the code by using TransactionChain.fetch_genesis_address_remotely(address) fixes the issue.
Do you have any idea why or what I might be doing wrong?

Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on lines 43 to 52
@desc """
Query the network to find the genesis address of a transaction
"""
field :get_genesis_address, :genesis_address do
arg(:address, non_null(:address))

resolve(fn %{address: address}, _ ->
Resolver.get_genesis_address(address)
end)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

this part need to be tested, you can have a look at test/archethic_web/graphql_schema_test.exs

@samuelmanzanera
Copy link
Member

Also, I think you could rename it as genesisAddress , because the entire graphQL API is readonly. Hence, the get could be omitted I guess.

@samuelmanzanera samuelmanzanera added the API Involve API facing user label Jan 5, 2023
Copy link
Contributor Author

@herissondev herissondev left a comment

Choose a reason for hiding this comment

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

This is a fix to Archethic.fetch_genesis_address_remotly which does not return a correct map when genesis_address is matched.

see 871f952

@desc """
Query the network to find the genesis address of a transaction
"""
field :genesis_address, :genesis_address do
Copy link
Member

Choose a reason for hiding this comment

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

Why create another type for the genesis address ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably not needed indeed, will update it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why create another type for the genesis address ?

Actually, I think it is needed. This query only returns the genesis address so I have to create a new type to only return the map genesis: address. Or am I getting this wrong ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks to @Neylix I was able to understand what was meant to do. The genesisAddress type was removed.

Copy link
Member

@bchamagne bchamagne left a comment

Choose a reason for hiding this comment

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

neat!

@samuelmanzanera samuelmanzanera merged commit ec9f419 into archethic-foundation:develop Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Involve API facing user external contribution Contribution by non core team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants