-
Notifications
You must be signed in to change notification settings - Fork 800
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
base: master
Are you sure you want to change the base?
Conversation
Actually , i'm confuse that my request is accept or not , because the testcase are run as i expected voting power. |
addresses.forEach((address: string, i: number) => { | ||
const balance = balances[i]; | ||
for (let j = 0; j < balance; j++) { | ||
tokenCalls.push([options.address, 'tokenOfOwnerByIndex', [address, j]]); | ||
} | ||
}); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please
There was a problem hiding this comment.
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 .
Please @ChaituVR , check the updated code. |
It should pass all test cases @monish-nagre
|
Fixes # .
Changes proposed in this pull request:
Types Of Land :
Mega contributes 25000 VP
Large contributes 10000 VP
Medium contributes 4000 VP
Unit contributes 2000 VP