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

Support RPC 0.8.0 #1510

Draft
wants to merge 21 commits into
base: development
Choose a base branch
from
Draft

Conversation

franciszekjob
Copy link
Collaborator

@franciszekjob franciszekjob commented Oct 29, 2024

Addresses #1498

Introduced changes

Support RPC 0.8.0.

  • starknet_getStorageProof
    • Add StorageProofResponse, GlobalRoots, ContractsProof, ContractLeafData, NodeHashToNodeMappingItem, BinaryNode, EdgeNode, ContractStorageKeys data classes
    • Add get_storage_proof method in FullNodeClient and Client
  • starknet_getMessagesStatus
    • Add MessageStatus data class
    • Add get_messages_status method in FullNodeClient and Client
  • Adapt execution resources to new receipt
    • Change l1_resource_bounds param to resource_bounds in tx-related methods and classes constructors
    • EstimatedFee:
      • rename gas_consumed to l1_gas_consumed
      • rename gas_price to l1_gas_price
      • rename data_gas_price to l1_data_gas_price
      • rename data_gas_consumed to l1_data_gas_consumed
      • add l2_gas_consumed, l2_gas_price fields
    • Remove ComputationResources and update ExecutionResources
    • Rename DataResources to InnerCallExecutionResources
    • Rename computation_resources to execution_resources in FunctionInvocation and change its type to InnerCallExecutionResources instead of ComputationResources
  • Add failure_reason to TransactionStatusResponse
  • starknet_getCompiledCasm
  • WebSockets
    • TODO: addressed in incoming PR

  • This PR contains breaking changes
  • Not compatible with JSON-RPC 0.7.0 spec
  • Use resource_bounds instead of l1_resource_bounds param
  • ComputationResources has been removed; ExecutionResources and StarknetFeeEstimate have been updated

@franciszekjob franciszekjob changed the title Franciszekjob/1498 rpc 0.8.0 Support RPC 0.8.0 Oct 29, 2024
- 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
Copy link
Collaborator Author

@franciszekjob franciszekjob Oct 30, 2024

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.

@franciszekjob
Copy link
Collaborator Author

franciszekjob commented Oct 30, 2024

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],
Copy link
Collaborator

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

Copy link
Collaborator Author

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:
Copy link
Collaborator

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?

Copy link
Collaborator Author

@franciszekjob franciszekjob Nov 1, 2024

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).

Comment on lines 621 to 628
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`.
Copy link
Collaborator

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.

Copy link
Collaborator Author

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]
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about children_hashes?

Copy link
Collaborator Author

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,
Copy link
Collaborator

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?

Copy link
Collaborator Author

@franciszekjob franciszekjob Oct 31, 2024

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)?

starknet_py/tests/e2e/account/account_test.py Outdated Show resolved Hide resolved
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