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

Improve GetFirstPublicKey message #276

Closed
ghost opened this issue Apr 8, 2022 · 7 comments
Closed

Improve GetFirstPublicKey message #276

ghost opened this issue Apr 8, 2022 · 7 comments
Assignees
Labels
feature New feature request good first issue Good for newcomers P2P Involve P2P networking

Comments

@ghost
Copy link

ghost commented Apr 8, 2022

GetFirstPublicKey implementation is not clear about the behavior expected and how it's used.
Actually it should load the first public key, but finally its implementation:

def process(%GetFirstPublicKey{address: address}) do
case TransactionChain.get_first_transaction(address, [:previous_public_key]) do
{:ok, %Transaction{previous_public_key: key}} ->
%FirstPublicKey{public_key: key}
{:error, :transaction_not_exists} ->
%NotFound{}
end
end

We are returning the first transaction public key or not found, but I think you should leverage more:

@doc """
Get the first public key from one the public key of the chain
"""
@spec get_first_public_key(Crypto.key()) :: Crypto.key()
defdelegate get_first_public_key(previous_public_key), to: DB, as: :get_first_public_key

So we would not have not found return, but the just public key passed in case there are not previous public key.

As we are messing with first transaction and first public key concept.
Moreover, we are fetching the transaction data and fields, which will cause more I/O than just fetching the first public key from the indexing.


Related to #268

@ghost ghost added feature New feature request P2P Involve P2P networking labels Apr 8, 2022
@internet-zero
Copy link
Member

Hey team! Please add your planning poker estimate with ZenHub @blackode @imnik11 @roychowdhuryrohit-dev @samuel-uniris

@internet-zero
Copy link
Member

Please add your planning poker estimate with ZenHub @apoorv-2204

@ghost ghost added the good first issue Good for newcomers label Apr 8, 2022
@prix-uniris
Copy link
Contributor

prix-uniris commented Apr 11, 2022

@samuel-uniris, @blackode
So, should i use only the Indexer to fetch details as per new db impl.

@spec get_first_public_key(Crypto.key(), String.t()) :: Crypto.key()
def get_first_public_key(public_key, db_path) do
# We derive the previous address from the public key to get the genesis address
# and its relative file
address = Crypto.derive_address(public_key)
genesis_address = get_first_chain_address(address, db_path)
filepath = chain_keys_path(db_path, genesis_address)
case File.open(filepath, [:binary, :read]) do
{:ok, fd} ->
# We need to extract key metadata information to know how many bytes to decode
# as keys can have different sizes based on the curve used
with {:ok, <<_timestamp::32, curve_id::8, origin_id::8>>} <- :file.read(fd, 6),
key_size <- Crypto.key_size(curve_id),
{:ok, key} <- :file.read(fd, key_size) do
# We then take the first public key registered
:file.close(fd)
<<curve_id::8, origin_id::8, key::binary>>
else
:eof ->
:file.close(fd)
public_key
end
{:error, _} ->
public_key
end
end

Using this function

@ghost
Copy link
Author

ghost commented Apr 11, 2022

@samuel-uniris So, should i use only the Indexer to fetch details as per new db impl.

@spec get_first_public_key(Crypto.key(), String.t()) :: Crypto.key()
def get_first_public_key(public_key, db_path) do
# We derive the previous address from the public key to get the genesis address
# and its relative file
address = Crypto.derive_address(public_key)
genesis_address = get_first_chain_address(address, db_path)
filepath = chain_keys_path(db_path, genesis_address)
case File.open(filepath, [:binary, :read]) do
{:ok, fd} ->
# We need to extract key metadata information to know how many bytes to decode
# as keys can have different sizes based on the curve used
with {:ok, <<_timestamp::32, curve_id::8, origin_id::8>>} <- :file.read(fd, 6),
key_size <- Crypto.key_size(curve_id),
{:ok, key} <- :file.read(fd, key_size) do
# We then take the first public key registered
:file.close(fd)
<<curve_id::8, origin_id::8, key::binary>>
else
:eof ->
:file.close(fd)
public_key
end
{:error, _} ->
public_key
end
end

Using this function

I guess you can use EmbeddedImpl.get_first_public_key​(​public_key​)​

@prix-uniris
Copy link
Contributor

So Instead of using TransactionChain.get_first_transaction Here:

def process(%GetFirstPublicKey{address: address}) do
case TransactionChain.get_first_transaction(address, [:previous_public_key]) do
{:ok, %Transaction{previous_public_key: key}} ->
%FirstPublicKey{public_key: key}
{:error, :transaction_not_exists} ->
%NotFound{}
end
end

Which eventually calls
DB.get_first_chain_address()
|> DB.get_first_chain_address()

So we should use
TransactionChain.get_first_public_key() which calls
DB.get_first_public_key() which eventually calls
DB.get_first_chain_address()

So, what is required to do for this Issue.
@samuel-uniris @blackode @apoorv-2204

@prix-uniris
Copy link
Contributor

Also not exactly sure , what "As we are messing with first transaction and first public key concept." this meant.

@apoorv-2204
Copy link
Contributor

As we are messing with first transaction and first public key concept.
@prix-uniris

image ,

Thanks @blackode for verifying the diagram

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature request good first issue Good for newcomers P2P Involve P2P networking
Projects
None yet
Development

No branches or pull requests

3 participants