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

eth_getLogs #103

Merged
merged 12 commits into from
Jun 1, 2022
Merged

eth_getLogs #103

merged 12 commits into from
Jun 1, 2022

Conversation

Ivo-Yankov
Copy link
Collaborator

@Ivo-Yankov Ivo-Yankov commented May 26, 2022

Description:

Related issue(s):

Fixes #
#20

Notes for reviewer:

Checklist

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

Signed-off-by: Ivo Yankov <[email protected]>
# Conflicts:
#	packages/relay/src/index.ts
#	packages/relay/src/lib/eth.ts
#	packages/relay/tests/lib/eth.spec.ts
# Conflicts:
#	packages/relay/src/index.ts
#	packages/relay/src/lib/model.ts
#	packages/server/tests/server.spec.ts
@Ivo-Yankov Ivo-Yankov changed the title feat: implement eth_getLogs eth_getLogs May 27, 2022
@Ivo-Yankov Ivo-Yankov added enhancement New feature or request P2 labels May 27, 2022
@Ivo-Yankov Ivo-Yankov linked an issue May 27, 2022 that may be closed by this pull request
@codecov-commenter
Copy link

codecov-commenter commented May 27, 2022

Codecov Report

Merging #103 (8e894fa) into main (e12eff6) will increase coverage by 5.68%.
The diff coverage is 93.84%.

@@            Coverage Diff             @@
##             main     #103      +/-   ##
==========================================
+ Coverage   60.84%   66.53%   +5.68%     
==========================================
  Files           4        4              
  Lines         424      490      +66     
  Branches       56       66      +10     
==========================================
+ Hits          258      326      +68     
+ Misses        151      147       -4     
- Partials       15       17       +2     
Impacted Files Coverage Δ
packages/relay/src/lib/eth.ts 51.43% <90.69%> (+6.61%) ⬆️
packages/relay/src/lib/clients/mirrorNodeClient.ts 97.22% <100.00%> (+9.46%) ⬆️
packages/relay/src/lib/model.ts 73.73% <100.00%> (+2.95%) ⬆️

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 e12eff6...8e894fa. Read the comment docs.

@Ivo-Yankov Ivo-Yankov marked this pull request as ready for review May 27, 2022 16:06
@Nana-EC Nana-EC added this to the 0.1.0 milestone May 27, 2022
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.

Looking good.
Some improvement suggestion of result build and request for 2 minor non happy path tests

packages/relay/src/lib/eth.ts Outdated Show resolved Hide resolved
packages/relay/src/lib/eth.ts Outdated Show resolved Hide resolved
packages/relay/tests/lib/eth.spec.ts Show resolved Hide resolved
@Ivo-Yankov Ivo-Yankov requested a review from Nana-EC May 30, 2022 13:02
Nana-EC
Nana-EC previously approved these changes Jun 1, 2022
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.
One nit.


describe('eth_getLogs', async function () {

const expectLogData1 = (res) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: A few shared checks in here can be pulled out.

const expectLogData = (res) => {
      expect(res.blockHash).to.eq(defaultDetailedContractResults.block_hash);
      expect(res.blockNumber).to.eq(defaultDetailedContractResults.block_number);
      expect(res.removed).to.eq(false);
      expect(res.topics).to.exist;
      expect(res.transactionHash).to.eq(defaultDetailedContractResults.hash);
      expect(res.transactionIndex).to.eq(defaultDetailedContractResults.transaction_index);
    };

Then each expectLogDataX(res) can call expectLogData(res)

@sonarcloud
Copy link

sonarcloud bot commented Jun 1, 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 3 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Ivo-Yankov Ivo-Yankov merged commit c51e2ba into main Jun 1, 2022
@Ivo-Yankov Ivo-Yankov deleted the 20-eth-get-logs branch June 1, 2022 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request P2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement eth_getLogs
4 participants