-
Notifications
You must be signed in to change notification settings - Fork 81
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
Support RPC 0.8.0 #1510
base: development
Are you sure you want to change the base?
Support RPC 0.8.0 #1510
Conversation
- Starknet - `0.13.3 <https://docs.starknet.io/documentation/starknet_versions/version_notes/#version0.13.3>`_ | ||
- RPC - `0.8.0 <https://github.com/starkware-libs/starknet-specs/releases/tag/v0.8.0>`_ | ||
|
||
TODO (#1498): List changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: Migration guide will be updated once we confirm that all introduced changes are correct.
note: Tests will be addressed later, once devnet fully implements RPC 0.8.0. |
@abstractmethod | ||
async def get_storage_proof( | ||
self, | ||
block_id: Union[int, Hash, Tag], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that there is no parameter for block_id
in the spec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's present in the spec on branch v0.8.0
.
@@ -381,42 +381,22 @@ class TransactionFinalityStatus(Enum): | |||
|
|||
|
|||
@dataclass | |||
class DataResources: | |||
class InnerCallExecutionResources: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually need this class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I updated it in this commit 😅 , execution_resources
in FunctionInvocation
should be of type InnerCallExecutionResources
(ref to spec).
starknet_py/net/client_models.py
Outdated
Calculates L1 max amount as `l1_max_amount` = `overall_fee` / `l1_gas_price`, unless `l1_gas_price` is 0, | ||
then L1 max amount is 0. Calculates `l1_max_price_per_unit` as `l1_max_price_per_unit` = `l1_gas_price`. | ||
|
||
Then multiplies `max_amount` by `amount_multiplier` and `max_price_per_unit` by `unit_price_multiplier`. | ||
Calculates L2 max amount as `l2_max_amount` = `overall_fee` / `l2_gas_price`, unless `l2_gas_price` is 0, | ||
then L2 max amount is 0. Calculates `l2_max_price_per_unit` as `l2_max_price_per_unit` = `l2_gas_price`. | ||
|
||
Then multiplies L1 max amount and L2 max amount by `amount_multiplier` and `l1_max_price_per_unit` | ||
and `l2_max_price_per_unit` by `unit_price_multiplier`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks we no longer need to use this hacky method, as we now have all the necessary fields in EstimatedFee
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've also added todo for decreasing amount and unit price multipliers.
child: int | ||
|
||
|
||
MerkleNode = Union[BinaryNode, EdgeNode] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about children_hashes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I was referring to MerkleNode
on v0.8.0
branch, and children_hashes
are not present there anymore.
@@ -231,7 +237,7 @@ async def deploy_v3( | |||
unique: bool = True, | |||
constructor_args: Optional[Union[List, Dict]] = None, | |||
nonce: Optional[int] = None, | |||
l1_resource_bounds: Optional[ResourceBounds] = None, | |||
resource_bounds: Optional[ResourceBoundsMapping] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe having 2 parameters l1_resource_bounds
and l2_resource_bounds
would be more convenient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tbh I don't think it's good to pass all fields of data class separately.
Maybe we can set l1_gas
and l2_gas
as optional in ResourceBoundsMapping
(if not passed they will be zero)?
Addresses #1498
Introduced changes
Support RPC 0.8.0.
starknet_getStorageProof
StorageProofResponse
,GlobalRoots
,ContractsProof
,ContractLeafData
,NodeHashToNodeMappingItem
,BinaryNode
,EdgeNode
,ContractStorageKeys
data classesget_storage_proof
method inFullNodeClient
andClient
starknet_getMessagesStatus
MessageStatus
data classget_messages_status
method inFullNodeClient
andClient
l1_resource_bounds
param toresource_bounds
in tx-related methods and classes constructorsEstimatedFee
:gas_consumed
tol1_gas_consumed
gas_price
tol1_gas_price
data_gas_price
tol1_data_gas_price
data_gas_consumed
tol1_data_gas_consumed
l2_gas_consumed
,l2_gas_price
fieldsComputationResources
and updateExecutionResources
DataResources
toInnerCallExecutionResources
computation_resources
toexecution_resources
inFunctionInvocation
and change its type toInnerCallExecutionResources
instead ofComputationResources
failure_reason
toTransactionStatusResponse
starknet_getCompiledCasm
starknet_getCompiledCasm
#1514resource_bounds
instead ofl1_resource_bounds
paramComputationResources
has been removed;ExecutionResources
andStarknetFeeEstimate
have been updated