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

[erc721-with-landtype-multiplier] Add strategy erc721-with-landtype-multiplier #1529

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

monish-nagre
Copy link

Fixes # .

Changes proposed in this pull request:

  • This strategy returns the balances of the voters for a specific ERC721 NFT with an arbitrary multiplier based on the type of land they own.
    Types Of Land :
    Mega contributes 25000 VP
    Large contributes 10000 VP
    Medium contributes 4000 VP
    Unit contributes 2000 VP

@ChaituVR ChaituVR changed the title Add strategy erc721-with-landtype-multiplier [erc721-with-landtype-multiplier] Add strategy erc721-with-landtype-multiplier Jul 15, 2024
@monish-nagre
Copy link
Author

Actually , i'm confuse that my request is accept or not , because the testcase are run as i expected voting power.
the error is for example.json address range like 3 and etc , i think that doesn't effect my strategy.
Please conform me.
My request is ready to merge / having issue ?

test/strategy.test.ts Outdated Show resolved Hide resolved
src/strategies/erc721-with-landtype-multiplier/index.ts Outdated Show resolved Hide resolved
Comment on lines 47 to 52
addresses.forEach((address: string, i: number) => {
const balance = balances[i];
for (let j = 0; j < balance; j++) {
tokenCalls.push([options.address, 'tokenOfOwnerByIndex', [address, j]]);
}
});
Copy link
Member

Choose a reason for hiding this comment

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

if my balance is in millions, we will send million calls to this multicall? 😄 is there some other way?

Copy link
Author

Choose a reason for hiding this comment

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

My current maximum supply of ERC721 tokens is 5000 and the balance is unlikely to be high due to the expensive nature of the lands.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should throw some error if it crosses a certain number, else it may eat all memory, or if there is way to get total supply of the tokens, and if it goes beyond a certain number we can make this strategy to stop working

Copy link
Author

@monish-nagre monish-nagre Jul 15, 2024

Choose a reason for hiding this comment

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

Thanks for suppport ...

add abi : [ 'function totalSupply() external view returns (uint256)' ]

const MAX_SUPPLY_THRESHOLD = totalSupply; // Set max supply threshold dynamically

// Check if the number of calls exceeds the maximum threshold
if (tokenCalls.length > MAX_SUPPLY_THRESHOLD) {
throw new Error(Number of token calls (${tokenCalls.length}) exceeds the maximum threshold (${MAX_SUPPLY_THRESHOLD}));
}

may i add these changes , it's okay ? i mean is satisfied for strategy

Copy link
Member

Choose a reason for hiding this comment

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

I think we should set a static MAX_SUPPLY_THRESHOLD, also this may allow few users and block few users 😅

I think it would be better if we could:

  • At the beginning of the strategy, Get totalSupply of contract
  • If it is more than 10k or something a bit more
  • Return error
  • Else continue

Copy link
Author

Choose a reason for hiding this comment

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

Yes I Understood ,
sorry for above response : ( My current maximum supply of ERC721 tokens is 5000 and the balance is unlikely to be high due to the expensive nature of the lands. ) its not my current supply , is my max limit that the land mint i.e 5000.

In my contract , the maximum LAND MINTING supply is 5000, and currently we sell (land-count) is 150 .
Soo we should set the MAX_SUPPLY_THRESHOLD to 5000.

Copy link
Member

Choose a reason for hiding this comment

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

works

Copy link
Author

@monish-nagre monish-nagre Jul 16, 2024

Choose a reason for hiding this comment

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

Thanks for reply ...
soo now , should i push the updated code ?

when i remove the try catch , my testcase runs now :
Test strategy "erc721-with-landtype-multiplier" with example index 0
✓ Strategy name should be lowercase and should not contain any special char expect hyphen (1 ms)
✓ Strategy name should be same as in examples.json (1 ms)
✓ Addresses in example should be minimum 3 and maximum 20
✓ Must use a snapshot block number in the past (5979 ms)
✕ Strategy should run without any errors (916 ms)
✕ Should return an array of object with addresses (1 ms)
✕ Should take less than 10 sec. to resolve
✕ File examples.json should include at least 1 address with a positive score
✕ Returned addresses should be checksum addresses
✕ Voting power should not depend on other addresses (410 ms)

Test strategy "erc721-with-landtype-multiplier" with example index 0 (latest snapshot)
✓ Strategy should run without any errors (2155 ms)
✓ Should return an array of object with addresses (3 ms)

Test strategy "erc721-with-landtype-multiplier" with example index 0 (with 500 addresses)
○ skipped Should work with 500 addresses
○ skipped Should take less than 20 sec. to resolve with 500 addresses

Other tests with example index 0
○ skipped Check schema (if available) is valid with examples.json
○ skipped Check schema (if available) all arrays in all levels should contain items property
○ skipped Strategy should work even when strategy symbol is null

Others:
✓ Author in strategy should be a valid github username (331 ms)
✓ Version in strategy should be a valid string (1 ms)

Copy link
Member

Choose a reason for hiding this comment

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

Yes please

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for support ... @ChaituVR
Yes i pushed the updated code now .

src/strategies/erc721-with-landtype-multiplier/index.ts Outdated Show resolved Hide resolved
src/strategies/erc721-with-landtype-multiplier/index.ts Outdated Show resolved Hide resolved
@monish-nagre
Copy link
Author

Please @ChaituVR , check the updated code.

@ChaituVR
Copy link
Member

It should pass all test cases @monish-nagre

 call revert exception [ See: https://links.ethers.org/v5-errors-CALL_EXCEPTION ] (method="balanceOf(address)", data="0x", errorArgs=null, errorName=null, errorSignature=null, reason=null, code=CALL_EXCEPTION, version=abi/5.6.4)

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.

2 participants