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

ACN service registry address #680

Merged
merged 278 commits into from
Jun 22, 2022
Merged

ACN service registry address #680

merged 278 commits into from
Jun 22, 2022

Conversation

Karrenbelt
Copy link
Contributor

@Karrenbelt Karrenbelt commented Apr 11, 2022

Proposed changes

Working on the extended RegistrationStartupBehaviour behavior and associated e2e tests.

Currently managed to:

  • introduce new agent registration_start_up for testing specifically the RegistrationStartupBehaviour
  • retrieve service info (e.g. registered addresses) from ServiceRegistry contract deployed on our staging chain.
    NOTE: addresses do not exactly match those of agents: one-off still.
  • exchange messages (tendermint_url) using Tendermint protocol via ACN client connection to ACN node (registration_start_up/aea-config.yaml).
  • 4 agent system running, no more deadlock and connecting to the same ACN port (per aea-config.yaml)
  • 4 agent system with catchup: other agents not crashing anymore

Noteworthy changes:

  • service registry contract: verify_contract was validation against outdated bytecode. Now returning True by default to accommodate changes in our online protocol without having to continuously update this, added an error messages to logger to indicate as much
  • removed the deadlocking yield.
  • removed callback functionality for Tendermint config sharing request / response handler.

TODO:

  • sort out agent addresses misalignment in tests and service registry (not sure why still there - off by one)
  • include Tendermint server (via image being developed here Create a test case for Tendermint e2e tests #781), then make required calls: request, update, restart.

dependencies added:

    connections/valory/p2p_libp2p_client:0.1.0
    contracts/valory/service_registry:0.1.0 
    protocols/valory/acn:1.1.0
    protocols/valory/tendermint:0.1.0

NOTES:

  • valory/contracts/service_registry is already here in the consensus repo.
  • TestTendermintHandler and TestRegistrationStartupBehaviour tests fail because these were changed, please ignore.
  • altered agent termination logic in BaseTestEnd2EndNormalExecution and BaseTestEnd2EndAgentCatchup. No agent terminates before the entire test is ran (otherwise leads to failures in BaseTestEnd2EndAgentCatchup tests
  • now terminating process (agent) instead of only popping from the list (cause other agents to crash with some regularity)

Please see comments in #760

Fixes

If it fixes a bug or resolves a feature request, be sure to link to that issue.

Types of changes

What types of changes does your code introduce? (A breaking change is a fix or feature that would cause existing functionality and APIs to not work as expected.)
Put an x in the box that applies

  • Non-breaking fix (non-breaking change which fixes an issue)
  • Breaking fix (breaking change which fixes an issue)
  • Non-breaking feature (non-breaking change which adds functionality)
  • Breaking feature (breaking change which adds functionality)
  • Refactor (non-breaking change which changes implementation)
  • Messy (mixture of the above - requires explanation!)

Checklist

Put an x in the boxes that apply.

  • I have read the CONTRIBUTING doc
  • I am making a pull request against the main branch (left side). Also you should start your branch off our main.
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

@codecov
Copy link

codecov bot commented Apr 11, 2022

Codecov Report

Merging #680 (60a4e38) into main (e796833) will decrease coverage by 0.71%.
The diff coverage is 100.00%.

❗ Current head 60a4e38 differs from pull request most recent head e811c4f. Consider uploading reports for the commit e811c4f to get more accurate results

@@            Coverage Diff             @@
##             main     #680      +/-   ##
==========================================
- Coverage   97.91%   97.20%   -0.72%     
==========================================
  Files         250      229      -21     
  Lines       14121    13712     -409     
==========================================
- Hits        13827    13329     -498     
- Misses        294      383      +89     
Flag Coverage Δ
unittests 97.20% <100.00%> (-0.72%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/valory/protocols/abci/__init__.py 100.00% <ø> (ø)
packages/valory/protocols/contract_api/__init__.py 100.00% <ø> (ø)
packages/valory/protocols/http/__init__.py 100.00% <ø> (ø)
packages/valory/protocols/ledger_api/__init__.py 100.00% <ø> (ø)
...ges/valory/skills/abstract_round_abci/dialogues.py 100.00% <100.00%> (ø)
...ages/valory/skills/abstract_round_abci/handlers.py 100.00% <100.00%> (ø)
...ckages/valory/skills/abstract_round_abci/models.py 100.00% <100.00%> (ø)
...ages/valory/skills/apy_estimation_abci/handlers.py 100.00% <100.00%> (ø)
...ory/skills/apy_estimation_chained_abci/handlers.py 100.00% <100.00%> (ø)
...valory/skills/liquidity_provision_abci/handlers.py 100.00% <100.00%> (ø)
... and 73 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e4f17a...e811c4f. Read the comment docs.

@DavidMinarsch DavidMinarsch changed the base branch from ACN-client-connection to main April 13, 2022 00:23
@DavidMinarsch
Copy link
Contributor

It's indeed not ideal that we have to run the connection just because we load it. For now there's no way around it. But you can open an issue on open-aea to consider adding an "is_abstract" mode for connections too! (We already have it for skills.)

@DavidMinarsch DavidMinarsch changed the title [WIP] ACN service registry address [DO NOT MERGE - UNTIL ACN DONE] ACN service registry address Apr 15, 2022
@Karrenbelt Karrenbelt force-pushed the ACN-service-registry-address branch from 3a89128 to 87f46a2 Compare April 21, 2022 07:56
@Karrenbelt Karrenbelt force-pushed the ACN-service-registry-address branch from 87f46a2 to cd0c815 Compare April 21, 2022 08:52
packages/valory/skills/registration_abci/behaviours.py Outdated Show resolved Hide resolved
packages/valory/skills/registration_abci/behaviours.py Outdated Show resolved Hide resolved
if not successful:
return

yield from super().async_act()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

@Karrenbelt Karrenbelt Apr 21, 2022

Choose a reason for hiding this comment

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

why yield from super().async_act()? This might not be necessary, but I don't see no harm either. It will create the first payload and ensures that all agents reached consensus on the initial data (which now includes the tendermint config). I suppose you want me to remove the yield from super().async_act() ?

@Adamantios Adamantios mentioned this pull request Jun 21, 2022
10 tasks
@angrybayblade
Copy link
Contributor

angrybayblade commented Jun 22, 2022

I tried running a local deployment but I got this

  File "/usr/lib/python3.10/hashlib.py", line 123, in __get_builtin_constructor
    raise ValueError('unsupported hash type ' + name)
ValueError: unsupported hash type ripemd160
AEA key provided. Copying to agent.
Usage: aea add-key [OPTIONS] TYPE FILE
Try 'aea add-key --help' for help.

Error: Invalid value: File 'cosmos_private_key.txt' does not exist.
Failed to add cosmos key needed for libp2p connection

@Adamantios
Copy link
Collaborator

I tried running a local deployment but I got this

  File "/usr/lib/python3.10/hashlib.py", line 123, in __get_builtin_constructor
    raise ValueError('unsupported hash type ' + name)
ValueError: unsupported hash type ripemd160
AEA key provided. Copying to agent.
Usage: aea add-key [OPTIONS] TYPE FILE
Try 'aea add-key --help' for help.

Error: Invalid value: File 'cosmos_private_key.txt' does not exist.
Failed to add cosmos key needed for libp2p connection

You also need to run:

autonomy generate-key cosmos cosmos_private_key.txt
autonomy add-key cosmos cosmos_private_key.txt
autonomy issue-certificates

We do this in order for the tests to be platform independent. In Windows, even if the sleep time is set to 0, we still need to call the act wrapper.
@angrybayblade
Copy link
Contributor

You also need to run:

autonomy generate-key cosmos cosmos_private_key.txt
autonomy add-key cosmos cosmos_private_key.txt
autonomy issue-certificates

The error occurs while running autonomy generate-key cosmos cosmos_private_key.txt

@Adamantios
Copy link
Collaborator

Adamantios commented Jun 22, 2022

Windows 3.7 and 3.10 tests failed on last run because of network failure:

wget not installed. An error occurred during installation:
 The remote server returned an error: (500) Internal Server Error. Internal Server Error
wget package files install failed with exit code 1. Performing other installation steps.
The install of wget was NOT successful.

I will rerun them later.

Macos test failed because of #989.

# Conflicts:
#	Pipfile
#	docs/counter_example.md
#	docs/quick_start.md
#	packages/hashes.csv
#	packages/valory/agents/apy_estimation/aea-config.yaml
#	packages/valory/agents/apy_estimation_chained/aea-config.yaml
#	packages/valory/agents/counter_client/aea-config.yaml
#	packages/valory/agents/hello_world/aea-config.yaml
#	packages/valory/agents/oracle/aea-config.yaml
#	packages/valory/agents/register_reset/aea-config.yaml
#	packages/valory/agents/simple_abci/aea-config.yaml
#	packages/valory/agents/test_abci/aea-config.yaml
#	packages/valory/protocols/tendermint/protocol.yaml
#	packages/valory/skills/abstract_round_abci/skill.yaml
#	packages/valory/skills/apy_estimation_abci/skill.yaml
#	packages/valory/skills/apy_estimation_chained_abci/skill.yaml
#	packages/valory/skills/hello_world_abci/skill.yaml
#	packages/valory/skills/liquidity_provision_abci/skill.yaml
#	packages/valory/skills/liquidity_rebalancing_abci/skill.yaml
#	packages/valory/skills/oracle_abci/skill.yaml
#	packages/valory/skills/oracle_deployment_abci/skill.yaml
#	packages/valory/skills/price_estimation_abci/skill.yaml
#	packages/valory/skills/register_reset_abci/skill.yaml
#	packages/valory/skills/registration_abci/skill.yaml
#	packages/valory/skills/reset_pause_abci/skill.yaml
#	packages/valory/skills/safe_deployment_abci/skill.yaml
#	packages/valory/skills/simple_abci/skill.yaml
#	packages/valory/skills/test_abci/skill.yaml
#	packages/valory/skills/transaction_settlement_abci/skill.yaml
#	tox.ini
@DavidMinarsch DavidMinarsch changed the title [DO NOT MERGE] ACN service registry address ACN service registry address Jun 22, 2022
@DavidMinarsch DavidMinarsch merged commit 65dc9b8 into main Jun 22, 2022
@DavidMinarsch DavidMinarsch deleted the ACN-service-registry-address branch June 22, 2022 16:50
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.

6 participants