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

Feat/gas param #332

Merged
merged 4 commits into from
Nov 2, 2023
Merged

Conversation

kalote
Copy link
Contributor

@kalote kalote commented Oct 31, 2023

What kind of change does this PR introduce (bug fix, feature, docs update, ...)?

New gas configuration in the optional params setting:

const config = {
  ipfsGateway: 'https://YOUR-IPFS-GATEWAY/ipfs/',
  gas: 20_000_000, // optional, default is 2_000_000
};

const erc725 = new ERC725(schemas, address, RPC_URL, config);

I also removed the gasPrice from the constants.ts file as it was unused.

What is the current behaviour (you can also link to an open issue here)?

Fix #328

What is the new behaviour (if this is a feature change)?

Other information:

@codecov-commenter
Copy link

codecov-commenter commented Oct 31, 2023

Codecov Report

Merging #332 (0d0f725) into develop (122efa9) will increase coverage by 0.67%.
Report is 84 commits behind head on develop.
The diff coverage is 88.88%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@             Coverage Diff             @@
##           develop     #332      +/-   ##
===========================================
+ Coverage    83.04%   83.71%   +0.67%     
===========================================
  Files           18       18              
  Lines          979     1130     +151     
  Branches       218      255      +37     
===========================================
+ Hits           813      946     +133     
- Misses          91       98       +7     
- Partials        75       86      +11     
Files Coverage Δ
src/constants/constants.ts 100.00% <100.00%> (ø)
src/constants/interfaces.ts 100.00% <100.00%> (ø)
src/lib/detector.ts 96.00% <100.00%> (+8.50%) ⬆️
src/lib/getData.ts 89.79% <ø> (ø)
src/lib/provider-wrapper-utils.ts 89.47% <100.00%> (ø)
src/schemas/index.ts 100.00% <100.00%> (ø)
src/types/Method.ts 100.00% <100.00%> (ø)
src/lib/decodeMappingKey.ts 84.21% <60.00%> (-5.54%) ⬇️
src/provider/providerWrapper.ts 81.48% <90.47%> (-1.42%) ⬇️
src/index.ts 75.00% <72.72%> (-2.00%) ⬇️
... and 3 more

... and 4 files with indirect coverage changes

@karalabe
Copy link

I don't know anything about ts, so I've no idea what and how it works :D... but I would also recommend supporting setting the gas to 0 as a wildcard (or null or something) that would remove the call limit completely and let the node run with whatever cap it has itself.

@skimaharvey
Copy link
Collaborator

skimaharvey commented Nov 1, 2023

What kind of change does this PR introduce (bug fix, feature, docs update, ...)?

New gas configuration in the optional params setting:

const config = {
  ipfsGateway: 'https://YOUR-IPFS-GATEWAY/ipfs/',
  gas: 20_000_000, // optional, default is 2_000_000
};

const erc725 = new ERC725(schemas, address, RPC_URL, config);

I also removed the gasPrice from the constants.ts file as it was unused.

What is the current behaviour (you can also link to an open issue here)?

Fix #328

What is the new behaviour (if this is a feature change)?

Other information:

Also agree. 2_000_000 is so arbitrary. May be would exceed the gas block limit for some chains or would be irrelevant for others. Specially if there is no need to set it for a eth_call

@kalote
Copy link
Contributor Author

kalote commented Nov 2, 2023

So shall we set the default to 0? @CallumGrindle / @skimaharvey / @Hugoo ?

@kalote
Copy link
Contributor Author

kalote commented Nov 2, 2023

Default is 2_000_000 but can be change to 0 if needed via the param. I guess it can be added to the next release @CallumGrindle

@kalote
Copy link
Contributor Author

kalote commented Nov 2, 2023

Default gas value set to 1_000_000 (1Gwei).

@CallumGrindle CallumGrindle merged commit db96536 into ERC725Alliance:develop Nov 2, 2023
2 checks passed
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.

Weird forced gas limit on getData
6 participants