Skip to content

Fix unstaking flows #342

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

Merged
merged 11 commits into from
Aug 27, 2024
Merged

Fix unstaking flows #342

merged 11 commits into from
Aug 27, 2024

Conversation

jmoreira-valory
Copy link
Collaborator

@jmoreira-valory jmoreira-valory commented Aug 23, 2024

This PR contains several improvements in terms of updating the staking/unstaking flow:

  • Ensures that the agent bond on chain matches the selected staking contract.
  • Collects metadata from staking programs on-chain.
  • Fixes the staking/unstaking flow to match the discussions in the working document.
  • Fixes computation of balance on run_service.sh
  • Removes unused contract ABI JSONs from the folder /contracts and uses the files already available on the packages folder.
  • Removes the update_service.py script and uses the built-in autonomy command.

scripts/utils.py Outdated
@@ -69,7 +69,7 @@
GAS_PARAMS = {
"maxFeePerGas": 30_000_000_000,
"maxPriorityFeePerGas": 3_000_000_000,
"gas": 500_000,
"gas": 5_000_000,
Copy link
Collaborator Author

@jmoreira-valory jmoreira-valory Aug 24, 2024

Choose a reason for hiding this comment

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

This gives issues on Tenderly sometimes. Discuss if this should be increased

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@0xArdi "gas": 500_000 has caused issues sometimes on Tenderly when sending the unstake transaction. I suspect it usually runs out of gas due to being executed when several checkpoints have not been called (thus it needs to do extra work). This seems not happening on mainnet, where the checkpoint is periodically called. What do you suggest to do here?

Copy link
Contributor

@0xArdi 0xArdi Aug 26, 2024

Choose a reason for hiding this comment

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

Ideally we do this using automatic gas estimation. Let's keep it as-is for now, the middleware doesnt have this problem, and is supposed to be the long-term solution for this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, but in this script we are not doing it. So what should we do? keep it asi it is 500.000 ? increase it?

SERVICE_ID_PATH = Path(STORE_PATH, "service_id.txt")
PACKAGES_PATH = Path(SCRIPT_PATH, "..", "trader", "packages")

REGISTRY_JSON = PACKAGES_PATH / "valory" / "contracts" / "service_registry" / "build" / "ServiceRegistryL2.json"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Remove redundant file from contracts (ServiceRegistryL2.json)

@@ -61,7 +61,6 @@
DEFAULT_ON_CHAIN_INTERACT_SLEEP = 6.0


ZERO_ADDRESS = "0x0000000000000000000000000000000000000000"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Already imported

if [ "${USE_STAKING}" = true ]; then
poetry run python "../scripts/staking.py" "$service_id" "$CUSTOM_SERVICE_REGISTRY_ADDRESS" "$CUSTOM_STAKING_ADDRESS" "../$operator_pkey_path" "$rpc" "$unstake";
fi
# Unstake if applicable
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The script already takes care if the service is staked or not.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not used anymore. It was used when the mech events were retrieved through the RPC. Now they are retrieved through the Subgraph.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Already available on the packages folder of the service.

Comment on lines +676 to +681
# Display information of the Git repository
current_branch=$(git rev-parse --abbrev-ref HEAD)
latest_commit_hash=$(git rev-parse HEAD)
echo "Current branch: $current_branch"
echo "Commit hash: $latest_commit_hash"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Help identify commit user is running when users request help.

cost_of_bonding=$MIN_STAKING_BOND_XDAI
fi

if [ "$local_service_hash" != "$remote_service_hash" ] || [ "$on_chain_agent_id" != "$AGENT_ID" ] || [ "$on_chain_agent_bond" != "$cost_of_bonding" ]; then
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If hash, agent_id or bond don't match the expected values, it requires an on-chain service update.

Comment on lines +991 to +1008
cmd="poetry run autonomy mint \
--retries $RPC_RETRIES \
--timeout $RPC_TIMEOUT_SECONDS \
--use-custom-chain \
service packages/valory/services/trader/ \
--key \"../$operator_pkey_path\" $password_argument \
--nft $nft \
-a $AGENT_ID \
-n $n_agents \
--threshold $n_agents \
--update \"$service_id\""

if [ "${USE_STAKING}" = true ]; then
cost_of_bonding=$MIN_STAKING_BOND_OLAS
poetry run python "../scripts/update_service.py" "../$operator_pkey_path" "$nft" "$AGENT_ID" "$service_id" "$CUSTOM_OLAS_ADDRESS" "$cost_of_bonding" "packages/valory/services/trader/" "$rpc" $password_argument
cmd+=" -c $cost_of_bonding --token $CUSTOM_OLAS_ADDRESS"
else
cost_of_bonding=$MIN_STAKING_BOND_XDAI
cmd="poetry run autonomy mint \
--retries $RPC_RETRIES \
--timeout $RPC_TIMEOUT_SECONDS \
--use-custom-chain \
service packages/valory/services/trader/ \
--key \"../$operator_pkey_path\" $password_argument \
--nft $nft \
-a $AGENT_ID \
-n $n_agents \
-c $cost_of_bonding \
--threshold $n_agents \
--update \"$service_id\""
cmd+=" -c $cost_of_bonding"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Use the already available autonomy command (--token option was not available and a custom script was created)

Comment on lines +65 to +67
NO_STAKING_PROGRAM_ID: ZERO_ADDRESS,
"quickstart_beta_hobbyist": "0x389B46c259631Acd6a69Bde8B6cEe218230bAE8C",
"quickstart_beta_expert": "0x5344B7DD311e5d3DdDd46A4f71481bD7b05AAA3e",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Simplified this data structure - we only need the contract address. All remaining information can be fetch on-chain.

@jmoreira-valory
Copy link
Collaborator Author

Resolves #151

@jmoreira-valory jmoreira-valory linked an issue Aug 25, 2024 that may be closed by this pull request
@dagacha dagacha merged commit d79b74d into develop Aug 27, 2024
@Adamantios Adamantios deleted the fix/unstaking branch August 28, 2024 08:05
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.

Use CLI to update staking services
4 participants