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

Fix blocking behaviour on Pearl agent #271

Merged
merged 8 commits into from
Jun 17, 2024

Conversation

jmoreira-valory
Copy link
Collaborator

@jmoreira-valory jmoreira-valory commented Jun 10, 2024

This PR fixes several issues that prevent the Pearl agent starting normally.

  • Several fixes to the staking interaction.

    • Wrong contracts being called
    • Wrong computation of minimum number of mech requests
    • Issue with StakingState: duplicate class name in different packages
  • Remove override from service.yaml in trader_omen_gnosis (used in Perl)

    The reason for this change is that the environment exports of strategies_kwargs is causing issues in the definition of export variables on Perl; it is producing variables with a period, which is not a valid variable name in bash.

        "SKILL_TRADER_ABCI_MODELS_PARAMS_ARGS_STRATEGIES_KWARGS_0": "[\"bet_kelly_fraction\",1]",
        "SKILL_TRADER_ABCI_MODELS_PARAMS_ARGS_STRATEGIES_KWARGS_1": "[\"floor_balance\",500000000000000000]",
        "SKILL_TRADER_ABCI_MODELS_PARAMS_ARGS_STRATEGIES_KWARGS_2_0": "bet_amount_per_threshold",
        "SKILL_TRADER_ABCI_MODELS_PARAMS_ARGS_STRATEGIES_KWARGS_2_1_0.0": "0",
        "SKILL_TRADER_ABCI_MODELS_PARAMS_ARGS_STRATEGIES_KWARGS_2_1_0.1": "0",
        "SKILL_TRADER_ABCI_MODELS_PARAMS_ARGS_STRATEGIES_KWARGS_2_1_0.2": "0",
        "SKILL_TRADER_ABCI_MODELS_PARAMS_ARGS_STRATEGIES_KWARGS_2_1_0.3": "0",
        "SKILL_TRADER_ABCI_MODELS_PARAMS_ARGS_STRATEGIES_KWARGS_2_1_0.4": "0",
        "SKILL_TRADER_ABCI_MODELS_PARAMS_ARGS_STRATEGIES_KWARGS_2_1_0.5": "0",
        "SKILL_TRADER_ABCI_MODELS_PARAMS_ARGS_STRATEGIES_KWARGS_2_1_0.6": "60000000000000000",
        "SKILL_TRADER_ABCI_MODELS_PARAMS_ARGS_STRATEGIES_KWARGS_2_1_0.7": "80000000000000000",
        "SKILL_TRADER_ABCI_MODELS_PARAMS_ARGS_STRATEGIES_KWARGS_2_1_0.8": "160000000000000000",
        "SKILL_TRADER_ABCI_MODELS_PARAMS_ARGS_STRATEGIES_KWARGS_2_1_0.9": "1000000000000000000",
        "SKILL_TRADER_ABCI_MODELS_PARAMS_ARGS_STRATEGIES_KWARGS_2_1_1.0": "1000000000000000000",
    

@jmoreira-valory jmoreira-valory changed the title Remove override from service.yaml Staking fixes and remove override from service.yaml Jun 15, 2024
@jmoreira-valory jmoreira-valory changed the title Staking fixes and remove override from service.yaml Fix blocking behaviour on Pearl agent Jun 15, 2024
Comment on lines +108 to +113

# The class StakingState is redefined in several packages.
# This conversion is required to use a single representation.
if not isinstance(state, StakingState):
state = StakingState(state.value)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Review this approach

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Adamantios please validate this approach

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you elaborate? Why is this necessary? It is only specified in the contract from where you imported it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

service_staking_state is initialized with StakingState defined in

https://github.com/valory-xyz/trader/blob/main/packages/valory/contracts/service_staking_token/contract.py#L30

and

https://github.com/valory-xyz/trader/blob/feat/new-staking/packages/valory/contracts/staking_token/contract.py#L30

deppending of which of the 2 contracts is being used. These are 2 different classes and when service_staking_state is compared, it will fail if it's not the class defined in this file. For this reason, this patch here ensures that it always uses StakingState imported in this file.

Copy link
Collaborator

@Adamantios Adamantios Jun 17, 2024

Choose a reason for hiding this comment

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

So I think the best approach here would be to revert back to #209 (comment), i.e., revert 93b7dc8.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This branch has already been tested.
I added an issue to ammend on the base branch before merging to main. #280

@@ -79,7 +79,6 @@ models:
redeem_round_timeout: 3600.0
contract_timeout: 300.0
file_hash_to_strategies_json: ${FILE_HASH_TO_STRATEGIES_JSON:list:[["bafybeihufqu2ra7vud4h6g2nwahx7mvdido7ff6prwnib2tdlc4np7dw24",["bet_amount_per_threshold"]],["bafybeif55cu7cf6znyma7kxus4wxa2doarhau2xmndo57iegshxorivwmq",["kelly_criterion"]]]}
strategies_kwargs: ${STRATEGIES_KWARGS:list:[["bet_kelly_fraction",1],["floor_balance",500000000000000000],["bet_amount_per_threshold",{"0.0":0,"0.1":0,"0.2":0,"0.3":0,"0.4":0,"0.5":0,"0.6":60000000000000000,"0.7":80000000000000000,"0.8":160000000000000000,"0.9":1000000000000000000,"1.0":1000000000000000000}]]}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Adamantios This export is causing issues on Pearl when exporting variables. Perhaps open an issue on Open Autonomy. The approach to make it work is to remove the service override and use directly the agent configuration.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As long as it is not removed from the main service file it's fine. Yes, please open an issue too.

Copy link
Collaborator

@Adamantios Adamantios Jun 17, 2024

Choose a reason for hiding this comment

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

Are you sure you can even use overrides on Pearl? From #221 (comment) I understand that it is not possible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Pearls seems to be using a custom method for overrides, but it's failing for STRATEGIES_KWARGS.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So why are you using FILE_HASH_TO_STRATEGIES_JSON and STRATEGIES_KWARGS above?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These work fine. The problem with STRATEGIES_KWARGS is explained in the description of this PR. #271 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let me rephrase. Why are you not using overrides everywhere and effectively duplicating everything (#221 (review))? If the answer is that this approach does not work on Pearl (#221 (comment)), why are you using FILE_HASH_TO_STRATEGIES_JSON and STRATEGIES_KWARGS?

@jmoreira-valory jmoreira-valory merged commit c26b5be into feat/new-staking Jun 17, 2024
5 of 6 checks passed
@jmoreira-valory jmoreira-valory deleted the fix/exported_variables branch June 17, 2024 14:37
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