Skip to content
This repository was archived by the owner on Jan 26, 2023. It is now read-only.

Conversation

@fmorency
Copy link
Contributor

@fmorency fmorency commented Sep 30, 2022

Implements

  • blockchain.list
  • blockchain.request
  • blockchain.response

Still need to write tests for all of those, but some introspection tooling in many would be needed; I reaaaally don't want to do that using grep and sed.

I asked @tantommy to help with the testing.

Depends on liftedinit/many-rs#208

@fmorency fmorency added the enhancement New feature or request label Sep 30, 2022
@fmorency fmorency self-assigned this Sep 30, 2022
@fmorency fmorency linked an issue Sep 30, 2022 that may be closed by this pull request
@codecov-commenter
Copy link

Codecov Report

Merging #249 (737fd4d) into main (d30efc7) will decrease coverage by 1.08%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #249      +/-   ##
==========================================
- Coverage   76.85%   75.76%   -1.09%     
==========================================
  Files          43       43              
  Lines        7975     8089     +114     
==========================================
  Hits         6129     6129              
- Misses       1846     1960     +114     
Impacted Files Coverage Δ
src/many-abci/src/module.rs 0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@fmorency fmorency marked this pull request as ready for review October 18, 2022 21:22
@fmorency fmorency requested a review from hansl October 18, 2022 21:22
Copy link
Contributor

@hansl hansl left a comment

Choose a reason for hiding this comment

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

Can I have some tests please?


// The default query returns an error (TM 0.35)
// Return all blocks
// TODO: Test on TM 0.34 and report an issue in TM-rs if reproducible
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this TODO blocking this PR? If not, add an issue and link it 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.

Not blocking. I didn't have time to test it on 0.34; thus not opened an issue yet.

@fmorency
Copy link
Contributor Author

Can I have some tests please?

@hansl

See PR description above.

Still need to write tests for all of those, but some introspection tooling in many would be needed; I reaaaally don't want to do that using grep and sed.

@tantommy can you comment on the tests you made on your side?

Copy link
Contributor

@hansl hansl left a comment

Choose a reason for hiding this comment

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

Approving conditional to @tantommy approval too (since he's doing QA on this issue).

@tt0mmy
Copy link

tt0mmy commented Oct 24, 2022

i have tested these endpoints and they are working as expected. lgtm

@fmorency fmorency merged commit 4626717 into liftedinit:main Oct 24, 2022
@fmorency fmorency deleted the fmorency/enh-245-missing-blockchain-endpoints branch October 24, 2022 14:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add blockchain.request and blockchain.response API's

4 participants