feat(rpc): remove support for RPC versions lower and equal to v0.8#3474
feat(rpc): remove support for RPC versions lower and equal to v0.8#3474CHr15F0x wants to merge 10 commits into
Conversation
CHr15F0x
force-pushed
the
chris/rm-rpc-le-v0.8
branch
10 times, most recently
from
ac3ea0d to
b45982a
Compare
| "signature": [], | ||
| "contract_address": contract_address, | ||
| "nonce": nonce, | ||
| "sender_address": sender_address, |
There was a problem hiding this comment.
Looking at the JSON-RPC spec, didn't INVOKE_TXN_V3 use sender_address already in v08? How did this even work?
There was a problem hiding this comment.
Well, it didn't:
------------------------------------------------------------------------------
Name | # times run | # fails | trans/s | fail/s
------------------------------------------------------------------------------
1: v08_estimate_fee
1: | 1,372 | 1,372 (100%) | 1372 | 1372
There was a problem hiding this comment.
Rn it's a valid V1 transaction, which actually works.
It used to be a V0 transaction, wrongly marked as a V1.
| #[case::root_trace_websocket("/ws", "v09/starknet_trace_api_openrpc.json", &[], Api::WebsocketOnly)] | ||
| #[case::root_write("/", "v09/starknet_write_api.json", &[], Api::HttpOnly)] | ||
| #[case::root_write_websocket("/ws", "v09/starknet_write_api.json", &[], Api::WebsocketOnly)] | ||
| #[case::root_websocket( |
There was a problem hiding this comment.
Hmm, if case::root_api_websocket works with no excludes, why shouldn't this case? And isn't just one of them enough?
|
This PR is truly massive. Most of it looks correct but there's just too much to digest in one go, plus probably some small nuances about Starknet changes I cannot recall from memory. These I assume should be caught by the tests you're already running. A question I do have is about all the now hardcoded Wdyt? |
Good idea, thanks 🙏 . |
zvolin
left a comment
zvolin
left a comment
There was a problem hiding this comment.
first pass 😅 never had ff lagging that much
| if serializer.version < RpcVersion::V09 { | ||
| let parent_hash = self | ||
| .0 | ||
| .ok_or_else(|| crate::dto::Error::custom("Missing parent block hash"))?; | ||
| serializer.serialize_field("parent_hash", &parent_hash)?; | ||
| } |
There was a problem hiding this comment.
this whole impl could now be removed, no?
| pub fn get( | ||
| &self, | ||
| tx: &Transaction<'_>, | ||
| rpc_version: RpcVersion, |
| let failure_reason = match status { | ||
| TxStatus::Rejected { error_message, .. } => error_message, | ||
| _ => None, | ||
| }; |
There was a problem hiding this comment.
This seems like a bug, both the current version and removing it. Looking at the spec, failure_reason should be derived from execution_status (REVERTED), not finality status. And it should not be removed.
CHr15F0x
force-pushed
the
chris/rm-rpc-le-v0.8
branch
from
b45982a to
a963a7f
Compare
CHr15F0x
force-pushed
the
chris/rm-rpc-le-v0.8
branch
from
a963a7f to
b24cabd
Compare
5 participants
Fixes: #3476
I'd appreciate as many eyes as possible on this due to the sheer size of the delta.
I've removed the older specs too. I see no reason to keep them, apart from misleading the reader of the repo that they are still somewhat relevant in the current release.
Tests:
starknet.jsstarknet.py( 🟢 30, 🔴 4 investigating...)