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

Update spec reader tests #657

Open
wants to merge 47 commits into
base: json-rpc-v0.8.0
Choose a base branch
from

Conversation

marioiordanov
Copy link
Contributor

@marioiordanov marioiordanov commented Dec 3, 2024

Development related changes

CI is failing because libraries used are not updated: starknet-rs, starknet_api, blockifier

Checklist:

  • Checked out the contribution guidelines
  • Applied formatting - ./scripts/format.sh
  • No linter errors - ./scripts/clippy_check.sh
  • No unused dependencies - ./scripts/check_unused_deps.sh
  • No spelling errors - ./scripts/check_spelling.sh
  • Performed code self-review
  • Rebased to the latest commit of the target branch (or merged it into my branch)
    • Once you make the PR reviewable, please avoid force-pushing
  • Updated the docs if needed - ./website/README.md
  • Linked the issues resolvable by this PR - linking info
  • Updated the tests if needed; all passing - execution info

@marioiordanov marioiordanov marked this pull request as ready for review December 12, 2024 07:37
crates/starknet-devnet-core/src/starknet/events.rs Outdated Show resolved Hide resolved
crates/starknet-devnet-core/src/blocks/mod.rs Outdated Show resolved Hide resolved
.for_each(|(_, block_hash)| {
.for_each(|(block_number, block_hash)| {
if *block_number == pending_block_number {
insert_pending_block_in_final_result = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

When will it be false? Only when one of the blocks in num_to_hash has the same number as pending_block? And when should that be the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly in this case. num_to_hash apart from being in the same object as pending_block has no relation to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Creating pending block have to be changed. It should advance the block number, but this is happening not in StarknetBlocks file, but in Starknet. Thats why this case is covered

crates/starknet-devnet-core/src/blocks/mod.rs Outdated Show resolved Hide resolved
crates/starknet-devnet-types/src/rpc/block.rs Outdated Show resolved Hide resolved
}
}
}
}
}

fn deserialize_response_to_type_or_panic<T: DeserializeOwned>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this method needed? I.e. what does it introduce that isn't supported by serde already? Writing the error to a custom file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just extracts common logic

- action: CHANGE
what: DEPRECATED_CAIRO_ENTRY_POINT.properties.offset from $ref to type
- action: ADD
what: DoubleDeref tuple maximum 32767 to the integer part
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need to be done for DoubleDeref? Why doesn't it need to be done for CellRef?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is added to CellRef, but automatically. Check out add: section of this file.

@@ -56,19 +92,11 @@ edit_manually:
what: starknet_getBlockWithTxHashes -> result removed PENDING_BLOCK_WITH_TX_HASHES
- action: ADD
what: execution_status to required fields of the result of starknet_getTransactionStatus
- action: ADD
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was the order changed for some of the actions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants