Skip to content
This repository has been archived by the owner on Dec 15, 2023. It is now read-only.

Fix rpc get_events endpoint #395

Merged
merged 3 commits into from
Feb 15, 2023
Merged

Fix rpc get_events endpoint #395

merged 3 commits into from
Feb 15, 2023

Conversation

THenry14
Copy link
Contributor

Usage related changes

Fixed some problems with rpc get_events:

  • from_block and to_block are now accepted as block_hashes
  • fixed from_block and to_block format in payload, so it now reflects the rpc spec
  • fixed problems with response schema validation

There is still a probllem with (in my opinion) a bug in rpc schema regarding Felts (they are in "^0x0[a-fA-F0-9]{1,63}$" format, that is different to gateway), that's why I had to wrap test data with rpc_felt. From what I know, the discussion is already opened with starkware cairo team to resolve this.

Checklist:

  • Applied formatting - ./scripts/format.sh
  • No linter errors - ./scripts/lint.sh
  • Performed code self-review
  • Rebased to the last commit of the target branch (or merged it into my branch)
  • Documented the changes
  • Linked the issues which this PR resolves
  • Updated the tests
  • All tests are passing - ./scripts/test.sh

@FabijanC FabijanC self-requested a review February 10, 2023 15:34
@FabijanC
Copy link
Collaborator

FabijanC commented Feb 10, 2023

@THenry14 Thanks for the PR! As you can see, the testing workflows have failed. I will probably review the PR next week after you have fixed this.

@THenry14
Copy link
Contributor Author

THenry14 commented Feb 13, 2023

@THenry14 Thanks for the PR! As you can see, the testing workflows have failed. I will probably review the PR next week after you have fixed this.

Oh, I'm sorry for that - I have run the linter, but then rebased and missed one unused import. I have addressed that in the newest commit, the tests should pass now 👍

edit: I can see they failed on "Check out previous test metadata" step, not sure what it does and if it's connected to the code at all

@FabijanC
Copy link
Collaborator

@THenry14

I can see they failed on "Check out previous test metadata" step, not sure what it does and if it's connected to the code at all

Thanks for fixing the linter issue. What we have now is most likely a CircleCI issue, I will check it out.

Copy link
Collaborator

@FabijanC FabijanC left a comment

Choose a reason for hiding this comment

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

The CircleCI issue doesn't seem to be resolvable from my end. The downloading of previous test results (for splitting between workers) doesn't seem to be executed (file not present in a later check step). I tried writing a report on CircleCI's discussion forum only to get my post marked as spam for providing too many links 🤣 .

Anyway, thank you for raising the quality of Devnet's code, just left some minor comments.

starknet_devnet/blueprints/rpc/misc.py Outdated Show resolved Hide resolved
starknet_devnet/blueprints/rpc/misc.py Show resolved Hide resolved
test/rpc/test_data/get_events.py Outdated Show resolved Hide resolved
@THenry14 THenry14 requested review from mikiw and FabijanC and removed request for mikiw February 15, 2023 08:32
@FabijanC FabijanC merged commit 46de0a7 into 0xSpaceShard:master Feb 15, 2023
@FabijanC FabijanC mentioned this pull request Feb 15, 2023
8 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants