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

Support 'from' in 'eth_call' #375

Merged
merged 6 commits into from
Jul 25, 2022
Merged

Support 'from' in 'eth_call' #375

merged 6 commits into from
Jul 25, 2022

Conversation

shemnon
Copy link
Contributor

@shemnon shemnon commented Jul 22, 2022

Description:
When a from value is in the eth_call params, set it to the senderId.

Related issue(s):

Fixes #373

Notes for reviewer:

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

When a from value is in the eth_call params, set it to the senderId.

Signed-off-by: Danno Ferrin <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Jul 22, 2022

Codecov Report

Merging #375 (3e10ae6) into main (dad8a94) will increase coverage by 0.77%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main     #375      +/-   ##
==========================================
+ Coverage   62.21%   62.98%   +0.77%     
==========================================
  Files           9        9              
  Lines         868      870       +2     
  Branches      142      143       +1     
==========================================
+ Hits          540      548       +8     
+ Misses        290      282       -8     
- Partials       38       40       +2     
Impacted Files Coverage Δ
packages/relay/src/lib/clients/sdkClient.ts 6.54% <0.00%> (-0.13%) ⬇️
packages/relay/src/lib/eth.ts 67.25% <100.00%> (+2.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dad8a94...3e10ae6. Read the comment docs.

@shemnon shemnon requested a review from Nana-EC July 22, 2022 23:11
Signed-off-by: Danno Ferrin <[email protected]>
Signed-off-by: Danno Ferrin <[email protected]>
Signed-off-by: Danno Ferrin <[email protected]>
Copy link
Collaborator

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

LG.
Just expand the coverage on the added eth.spec.ts

}
);

const result = await ethImpl.call({
Copy link
Collaborator

Choose a reason for hiding this comment

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

For completeness of coverage this methods should be checking that the result is expected.
Given the return above i think the expected result would be 0x0 or 0x.

Should make this change for the 4 tests added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

const contract = new ethers.Contract(evmAddress, abi, accounts[0].wallet);

return contract;
return new ethers.Contract(evmAddress, abi, accounts[0].wallet);
};
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: new line since you're here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@Nana-EC Nana-EC added bug Something isn't working P1 labels Jul 23, 2022
@Nana-EC Nana-EC added this to In progress in Development via automation Jul 23, 2022
@Nana-EC Nana-EC added this to the 0.5.0 milestone Jul 23, 2022
Copy link
Collaborator

@Daniel-K-Ivanov Daniel-K-Ivanov left a comment

Choose a reason for hiding this comment

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

LGTM! We might want to resolve @Nana-EC 's comments and then merge :)

Signed-off-by: Danno Ferrin <[email protected]>
Signed-off-by: Danno Ferrin <[email protected]>
@sonarcloud
Copy link

sonarcloud bot commented Jul 25, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Collaborator

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

LGTM

@Nana-EC Nana-EC moved this from In progress to For Review in Development Jul 25, 2022
@Nana-EC Nana-EC merged commit 5fd35e3 into main Jul 25, 2022
Development automation moved this from For Review to Done Jul 25, 2022
@Nana-EC Nana-EC deleted the 373-eth_call-from branch July 25, 2022 22:46
Nana-EC pushed a commit that referenced this pull request Jul 25, 2022
* Support from in eth_call

When a from value is in the eth_call params, set it to the senderId.

Signed-off-by: Danno Ferrin <[email protected]>
@Nana-EC Nana-EC mentioned this pull request Jul 25, 2022
2 tasks
Nana-EC added a commit that referenced this pull request Jul 26, 2022
Cherry pick #375

When a from value is in the eth_call params, set it to the senderId.

Signed-off-by: Danno Ferrin <[email protected]>

Co-authored-by: Danno Ferrin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working P1
Projects
Development

Successfully merging this pull request may close these issues.

eth_call does not respect the from field all the time
4 participants