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

[staked-defi-balance] support for multiple contracts, including traditional staking & open staking across different networks #1149

Merged
merged 22 commits into from
May 10, 2023

Conversation

taha-abbasi
Copy link
Contributor

@taha-abbasi taha-abbasi commented May 3, 2023

Release Notes

staked-defi-balance v1.2.6

Improvements

  • No need to provide multiple params anymore: The strategy has been optimized to work with the new multi-staked-defi-balance strategy, reducing the complexity of the configuration.
  • Removed the need to provide methodABI: Users don't need to provide methodABI in the configuration anymore, making it simpler to use.
  • Better integration with multi-staked-defi-balance: The staked-defi-balance strategy has been improved to work seamlessly with the new multi-staked-defi-balance strategy.

Test Results

staked-defi-balance v1.2.6
image

staked-defi-balance v1.2.6 with 500 addresses
image

Release Notes: staked-defi-balance v1.1.1

Overview

In this release, we have updated the staked-defi-balance strategy to support multiple contracts, including traditional staking and open staking contracts across different blockchain networks. This enhancement allows users to efficiently check staked balances for various contracts by providing the necessary parameters, such as tokenContractAddress, network, snapshot, stakingContractAddress, and ABI.

Features

  1. Multi-contract support: The strategy now supports multiple contracts, allowing users to check staked balances across different staking mechanisms.
  2. Cross-chain compatibility: The strategy can now work with different blockchain networks, making it more versatile for various use cases.
  3. Improved error handling: The updated code includes better error handling to ensure smooth execution and accurate results.
  4. Efficient multicall usage: The strategy continues to utilize multicall for efficient querying of staked balances.
  5. Support for "latest" snapshot value: You can now use the "latest" snapshot value for each parameter and the root snapshot value. This feature enables the strategy to use the latest available snapshot when calculating staked balances, providing more up-to-date and relevant results.
  6. Improved schema validation for EVM addresses: We've updated the schema validation to ensure that tokenContractAddress and stakingPoolContractAddress fields contain valid EVM addresses using a regex pattern.

Changes

  • Updated the strategy code to use the getProvider function for passing network information from the provided parameters.
  • Refactored the code to support multiple contracts and networks.
  • Improved error handling within the strategy code.

Schema Updates

  • The snapshot field in the strategy's schema now accepts either an integer value or the string "latest".
  • The tokenContractAddress and stakingPoolContractAddress fields in the schema now use the regex pattern ^0x[a-fA-F0-9]{40}$ to validate that the provided addresses are valid EVM addresses.

Code Changes

The significant changes in the code for supporting multiple contracts on multiple chains are:

  1. Looping through the options to handle multiple contracts:
for (const params of options) {
  // ...
}
  1. Replacing the fixed network and provider parameters with the paramNetwork and getProvider(paramNetwork) to support multiple chains:
const paramNetwork = params.network.toString();

const stakes = await multicall(
  paramNetwork,
  getProvider(paramNetwork),
  [abi],
  stakingCalls,
  { blockTag: paramSnapshot }
);

These changes have improved the versatility and efficiency of the staked-defi-balance strategy, making it capable of handling multiple contracts and networks.

Test Results

image

image

@@ -15,40 +13,69 @@ export async function strategy(
options,
snapshot
Copy link
Member

Choose a reason for hiding this comment

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

Hi @taha-abbasi you should use snapshot to calculate voting power at the proposal block, else voting power will be unpredictable and can cause lot of issues on the proposal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ChaituVR thanks for the suggestions, so the strategy is using snapshot just not the snapshot at the root of examples.json. I left it there to pass the tests as I cannot modify the tests.

You can see snapshot being used here:

for (const params of options) {
    const paramNetwork = params.network.toString();
    const paramSnapshot =
      typeof params.snapshot === 'number' ? params.snapshot : 'latest';

This also allows "latest" as a value which is what will be utilized most of the time. The snapshot will be different per network defined in params, hence the root snapshot value is not suitable.

Comment on lines 7 to 49
{
"network": "42161",
"snapshot": 85322593,
"tokenContractAddress": "0xe685d3CC0be48BD59082eDe30C3b64CbFc0326e2",
"symbol": "cFRM",
"decimals": 18,
"minStakedBalance": "500000000000000000000000",
"stakingPoolContractAddress": "0xb4927895cbee88e651e0582893051b3b0f8d7db8",
"methodABI": [
{
"inputs": [
{
"internalType": "address",
"name": "id",
"type": "address"
},
{
"internalType": "address",
"name": "staker",
"type": "address"
}
],
"name": "stakeOf",
"outputs": [
{
"internalType": "uint256",
"name": "",
"type": "uint256"
}
],
"stateMutability": "view",
"type": "function"
}
]
},
{
"network": "56",
"snapshot": 27877333,
"tokenContractAddress": "0xaf329a957653675613D0D98f49fc93326AeB36Fc",
"symbol": "cFRM",
"decimals": 18,
"minStakedBalance": "500000000000000000000000",
"stakingPoolContractAddress": "0x35e15ff9ebb37d8c7a413fd85bad515396dc8008",
Copy link
Member

Choose a reason for hiding this comment

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

Instead of passing multiple items like this, better to use multiple strategies in your space :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion @ChaituVR but currently, there aren't strategies that can handle the use case. In the future, I plan to create staked-defi-balance-traditional-staking and staked-defi-balance-open-staking strategies then I can pass strategies here.

Until that is done, we need this use case where params can hold the info to run multi-chain and multi-contract checks from the same strategy. 🙌

Copy link
Member

Choose a reason for hiding this comment

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

Hi @taha-abbasi This could increase the server load. and others will refer to this strategy and ask us to allow passing multiple strategies into a single strategy, ideally, it should ideally accept one network and one snapshot You can use a strategy multiple times in your space so would be great if you can modify like that, thanks for understanding 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @ChaituVR Please see the updates that include a new strategy per your suggestions:

Release Notes

staked-defi-balance v1.2.6

Improvements

  • No need to provide multiple params anymore: The strategy has been optimized to work with the new multi-staked-defi-balance strategy, reducing the complexity of the configuration.
  • Removed the need to provide methodABI: Users don't need to provide methodABI in the configuration anymore, making it simpler to use.
  • Better integration with multi-staked-defi-balance: The staked-defi-balance strategy has been improved to work seamlessly with the new multi-staked-defi-balance strategy.

multi-staked-defi-balance v1.0.0

New Features

  • Multi-strategy support: multi-staked-defi-balance allows users to pass multiple strategies instead of passing complex params with methodABIs, making it more efficient and simpler to use.
  • Cumulative staked balance calculation: The strategy calculates the cumulative staked balance across multiple staking contracts and networks for each user.
  • Flexible configuration: Users can configure multiple staking contracts across different networks and staking types (open and standard) in a single strategy.

Parameters

  • symbol: The symbol for the cumulative staked balance.
  • cumulativeMinimumStakedBalance: The minimum cumulative staked balance required for a user to pass the strategy.
  • strategies: An array of staked-defi-balance strategy objects, each representing a different staking contract configuration.

For more information about the parameters and usage, please refer to the README.md.

Compatibility

  • The multi-staked-defi-balance strategy is fully compatible with the staked-defi-balance strategy v1.2.6, and it's recommended to use them together for a better user experience.

We hope that these updates make the staked-defi-balance and multi-staked-defi-balance strategies more efficient and easier to use. As always, contributions, suggestions, and bug reports are welcome! Please submit them through our GitHub repository.

Test Results

staked-defi-balance
image

staked-defi-balance with 500 addresses
image

multi-staked-defi-balance
image

multi-staked-defi-balance with 500 addresses
image

I hope this satisfies the feedback, looking forward to hearing from you and the merge.

@taha-abbasi taha-abbasi requested a review from ChaituVR May 3, 2023 18:01
@taha-abbasi
Copy link
Contributor Author

@ChaituVR bumping this request, hope you can merge it today 🙏

@ChaituVR
Copy link
Member

Sorry if i am not clear, i mean you can use multiple staked-defi-balance strategies in your space, you don't need a seperate strategy :)

@taha-abbasi
Copy link
Contributor Author

taha-abbasi commented May 10, 2023

Sorry if i am not clear, i mean you can use multiple staked-defi-balance strategies in your space, you don't need a seperate strategy :)

@ChaituVR staked-defi-balance won't check cumulative balance across chains if it is being used multiple times in the space. Or am I missing something?

For example, if I want to ensure the cumulativeStakedBalance of a user on and openStakingContract on bsc + openStakingContract on arbitrum + standardStakingContract on arbitrum > x then they can create proposals, I don't think using multiple staked-defi-balance in one space will solve that.

Can you clarify if I am incorrect in the assumption? If I am not, that is why we need multi-staked-defi-balance which gather's the scores and returns only addresses that meet the cumulativeStakedBalance minimum.

@ChaituVR
Copy link
Member

Chaitu, [11/05/23 1:21 AM]
Hey Taha, adding multiple strategies with different networks will actually return a cumulative score of them

for > x to create proposals you can use proposal validation :)
for > x to vote, you can use voting validation

@taha-abbasi
Copy link
Contributor Author

Chaitu, [11/05/23 1:21 AM] Hey Taha, adding multiple strategies with different networks will actually return a cumulative score of them

for > x to create proposals you can use proposal validation :) for > x to vote, you can use voting validation

Excellent, thanks for clarifying that @ChaituVR . I have submitted the changes with multi-staked-defi-balance removed and staked-defi-balance now more optimized. The params no longer require ABIs in params and made it more efficient and simpler where it uses snapshot and network values.

@taha-abbasi taha-abbasi requested a review from ChaituVR May 10, 2023 21:40

// Filter out addresses that have a total staked balance less than the minStakedBalance
const minStakedBalance = parseFloat(
formatUnits(options[0].minStakedBalance, options[0].decimals)
Copy link
Member

Choose a reason for hiding this comment

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

here it checks only first params minStakedBalance @taha-abbasi

Copy link
Contributor Author

@taha-abbasi taha-abbasi May 10, 2023

Choose a reason for hiding this comment

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

The minimumBalanceRequirement is the same in each contract based on how this strategy will be used so no need to check each one. @ChaituVR

@ChaituVR ChaituVR merged commit 3e808f4 into snapshot-labs:master May 10, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants