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

Update felt schema to be less strict #412

Closed
wants to merge 4 commits into from

Conversation

fracek
Copy link

@fracek fracek commented Feb 27, 2023

Usage related changes

Update the schema used to validate felts to the most recent version
included in starknet-specs.
This schema is less strict, and allows felts that are not zero-padded.

Fix #401

Development related changes

No changes

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

Update the schema used to validate felts to the most recent version
included in starknet-specs.
This schema is less strict, and allows felts that are not zero-padded.
@fracek
Copy link
Author

fracek commented Feb 27, 2023

Looks like the response validation doesn't like the regex I added (copied from starknet-specs) and returns an error for every request.

{
	"error": {
		"code": -32603,
		"message": "Devnet tried to return invalid value: \"Unevaluated properties are not allowed ('block_hash', 'block_number', 'new_root', 'parent_hash', 'sequencer_address', 'status', 'timestamp', 'transactions' were unexpected)\""
	},
	"id": 0,
	"jsonrpc": "2.0"
}

@penovicp
Copy link

I can see the regex used was based on the recent change in spec, however, it looks like it might be missing some parentheses. ^0x(0|[a-fA-F1-9]{1}[a-fA-F0-9]{0,62}) checks for 0x followed by 0 or the rest, the intent was probably for it to be ^0x(0|[a-fA-F1-9]{1})([a-fA-F0-9]{0,62})$ so it checks for 0x followed by 0|[a-fA-F1-9]{1} followed by [a-fA-F0-9]{0,62}.

I'm unsure if the regex should maybe be ^0x0?[a-fA-F0-9]{1,63}$ so the leading zero is only optional and doesn't affect the count of significant digits.

@ClementWalter
Copy link

@penovicp see my correponding PR #413 with the ? which seems to me the most easy and straighforward change

@FabijanC
Copy link
Collaborator

FabijanC commented Feb 28, 2023

@ClementWalter @penovicp
Doesn't ^0x0?[a-fA-F0-9]{1,63}$ then allow 64 digits after 0x? Should that be considered legal?

EDIT: Sorry, that's how it's supposed to be, I had a wrong number in my head

@FabijanC
Copy link
Collaborator

FabijanC commented Feb 28, 2023

Personally I'm leaning towards penovicp's idea implement in Clement's #413

@fracek
Copy link
Author

fracek commented Feb 28, 2023

Agree! Let's keep it simple!

@fracek fracek closed this Feb 28, 2023
@FabijanC
Copy link
Collaborator

As pointed out here, we should reopen this PR and close #413.

@fracek Would you know how to resolve the issue arising with this PR?

@fracek
Copy link
Author

fracek commented Feb 28, 2023

The error originates when validating the response of calling starknet_getBlockWithTxs on the genesis block. I haven't investigated too much why it happens.

@FabijanC
Copy link
Collaborator

@mikiw Can you take a look

@mikiw
Copy link
Contributor

mikiw commented Mar 1, 2023

The error originates when validating the response of calling starknet_getBlockWithTxs on the genesis block. I haven't investigated too much why it happens.

I'll take a look now.

@mikiw
Copy link
Contributor

mikiw commented Mar 1, 2023

============================================ short test summary info =============================================
FAILED test/rpc/test_rpc_blocks.py::test_get_block_with_tx_hashes[hash] - KeyError: 'result'
FAILED test/rpc/test_rpc_blocks.py::test_get_block_with_tx_hashes[number] - KeyError: 'result'
FAILED test/rpc/test_rpc_blocks.py::test_get_block_with_tx_hashes[tag] - KeyError: 'result'
FAILED test/rpc/test_rpc_blocks.py::test_get_block_with_tx_hashes[tag_pending] - KeyError: 'result'
FAILED test/rpc/test_rpc_blocks.py::test_get_block_with_tx_hashes_raises_on_incorrect_block_id[block_id1] - assert {'code': -326...]{0,62})$\'"'} == {'code': 24, ...ck not found'}
FAILED test/rpc/test_rpc_blocks.py::test_get_block_with_txs[hash] - KeyError: 'result'
FAILED test/rpc/test_rpc_blocks.py::test_get_block_with_txs[number] - KeyError: 'result'
FAILED test/rpc/test_rpc_blocks.py::test_get_block_with_txs[tag] - KeyError: 'result'
FAILED test/rpc/test_rpc_blocks.py::test_get_block_with_txs[tag_pending] - KeyError: 'result'
FAILED test/rpc/test_rpc_blocks.py::test_get_block_with_txs_raises_on_incorrect_block_id[block_id1] - assert {'code': -326...]{0,62})$\'"'} == {'code': 24, ...ck not found'}
FAILED test/rpc/test_rpc_blocks.py::test_get_block_transaction_count[hash] - KeyError: 'result'
FAILED test/rpc/test_rpc_blocks.py::test_get_block_transaction_count_raises_on_incorrect_block_id[block_id1] - assert {'code': -326...]{0,62})$\'"'} == {'code': 24, ...ck not found'}
==================================== 12 failed, 7 passed in 136.81s (0:02:16) ====================================

I got these test results. Checking how to fix it.

@mikiw
Copy link
Contributor

mikiw commented Mar 1, 2023

In tests we use dummy data like 0x1 I advise disabling validation in those tests to avoid many changes from 0x1 to 0x000000000000000000000000000000000000000000000000000000000000001. So validation tests will only be in test_rpc_schema.py.

@mikiw
Copy link
Contributor

mikiw commented Mar 1, 2023

I made a 2fecdd9 commit with some test fixes and now just 3 tests are failing in this test file.

@mikiw
Copy link
Contributor

mikiw commented Mar 1, 2023

Actually, I reverted that, seems that fix can be made only by modifying the rpc_felt wrapper.

@mikiw
Copy link
Contributor

mikiw commented Mar 1, 2023

Actually, I tried that but only one test was fixed. This was my change:
image

    if value == 0:
        return f"{0:#0{65}x}"
    return f"{value:#0{65}x}"

Any ideas?

@FabijanC
Copy link
Collaborator

Superseded by #467

@FabijanC FabijanC closed this May 22, 2023
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.

RPC schema felt validation
5 participants