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

request missing address while receiving new last address #748

Conversation

tenmoves
Copy link
Contributor

@tenmoves tenmoves commented Dec 7, 2022

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes #713

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Unit tests to do

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

@tenmoves tenmoves added bug Something isn't working DB Involve database core team Assigned to the core team labels Dec 7, 2022
@tenmoves tenmoves self-assigned this Dec 7, 2022
@tenmoves tenmoves marked this pull request as ready for review December 8, 2022 09:37
Contracts.stop_contract(local_last_address)
end

process_notify_last_transaction_address(notify_last_tx_address_msg)
Copy link
Member

Choose a reason for hiding this comment

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

Why removing the implementation from the message processing as it is used only here ?

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 wanted to be able to unit test this implementation, and to keep the code simple

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 couldn't find test for how the message get processed, do we need to test this or it should be incorporated in a e2e test somewhere else ?

Copy link
Member

Choose a reason for hiding this comment

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

Send message is mocked, but you can do something like Mocking the message, and in the mock just call Message.process and it will run the process function on the right message

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

}) do
with {local_last_address, _} <- TransactionChain.get_last_address(genesis_address),
true <- local_last_address != last_address do
{:ok, tx} = TransactionChain.get_transaction(last_address)
Copy link
Member

Choose a reason for hiding this comment

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

If you receive this message, it means that you didn't replicate the last transaction, so using TransactionChain.get_transaction to fetch the transaction from DB will always fail.
I think the previous address has to be added in the message itself, because nodes which sent this message know 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.

yes it makes sens, I will do that thank you

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

@tenmoves tenmoves requested a review from Neylix December 14, 2022 09:05
@samuelmanzanera samuelmanzanera merged commit 0ea44ae into archethic-foundation:develop Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working core team Assigned to the core team DB Involve database
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request missing addresses while receiving a new last address
4 participants