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 #530

Draft
wants to merge 57 commits into
base: main
Choose a base branch
from
Draft

Support RPC 0.8.0 #530

wants to merge 57 commits into from

Conversation

franciszekjob
Copy link
Collaborator

@franciszekjob franciszekjob commented Oct 22, 2024

Describe your changes

Support RPC 0.8.0.

  • starknet_getStorageProof
    • Add StorageProof, GlobalRoots, ContractsProof, ContractLeafData, NodeHashToNodeMappingItem, BinaryNode, EdgeNode, ContractStorageKey data classes
    • Add getStorageProof method in Provider and JsonRpcProvider
  • starknet_getMessagesStatus
    • Add MessageStatusList, MessageStatus data classes
    • Add getMessagesStatus method in Provider and JsonRpcProvider
  • Adapt execution resources to new receipt
    • Account, StandardAccount:
      • signDeployAccountV3 and executeV3 have now resourceBounds param instead of l1ResourceBounds
    • InvokeParamsV3, DeclareParamsV3, DeployAccountParamsV3:
      • Change l1ResourceBounds param to resourceBounds in constructor
    • EstimateFeeResponse :
      • rename gasConsumed to l1GasConsumed
      • rename gasPrice to l1GasPrice
      • rename dataGasPrice to l1DataGasPrice
      • rename dataGasConsumed to l1DataGasConsumed
      • add l2GasConsumed, l2GasPrice fields
    • Remove ComputationResources and update ExecutionResources
    • Add InnerCallExecutionResources
    • Remove constructor accepting only l1Gas in ResourceBoundsMapping
    • Rename computationResources to executionResources in FunctionInvocation and change its type to InnerCallExecutionResources instead of ComputationResources
  • Add failureReason to GetTransactionStatusResponse

Linked issues

Closes #519

Breaking changes

  • This issue contains breaking changes
  • Not compatible with JSON-RPC 0.7.0 spec
  • Use resourceBounds instead of l1ResourceBounds param
  • ComputationResources has been removed; ExecutionResources and EstimateFeeResponse have been updated

@franciszekjob franciszekjob changed the title Feat/519 rpc 0.8.0 Support RPC 0.8.0 Oct 22, 2024
Comment on lines 665 to 677
/**
* Get merkle paths in one of the state tries.
*
* Get merkle paths in one of the state tries: global state, classes, individual contract.
*
* @param classHashes list of class hashes for which we want to prove membership
* @param contractAddresses list of contract addresses for which we want to prove membership
* @param contractsStorageKeys list of contract address and storage keys pairs
*
* @throws RequestFailedException
*/
fun getStorageProof(classHashes: List<Felt>? = null, contractAddresses: List<Felt>? = null, contractsStorageKeys: List<ContractStorageKey>? = null): Request<StorageProof>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

note: It's a bit exceptional situation, due to the fact that creating overloads would cause a conflict, e.g. for

fun getStorageProof(classHashes: List<Felt>, contractsStorageKeys: List<ContractStorageKey>): Request<StorageProof>

and

fun getStorageProof(contractAddresses: List<Felt>, contractsStorageKeys: List<ContractStorageKey>): Request<StorageProof>

(compiler won't know which method to use, contractAddresses and classHashes are of the same type). So unfortunately we need to add default values here.

@franciszekjob
Copy link
Collaborator Author

franciszekjob commented Oct 23, 2024

note: This PR is still wip.

  1. Tests will be addressed once devnet is not a blocker anymore.
  2. Full description about included changes will be added later.

Comment on lines 199 to 200
l1ResourceBounds: ResourceBounds,
l2ResourceBounds: ResourceBounds,
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should just go ahead and accept ResourceBoundsMapping in all of these methods?

Previously, there was no reason to do so. Now, my rationale is:

  1. It would simplify the signature
  2. These methods will probably be often used with the same values, so might as well reduce boilerplate
  3. When estimating fee prior to executing transactions, we can right away run EstimateFeeResponse.toResourceBounds and pass the result, transforming this:
            val resourceBounds = estimateFee.values.first().toResourceBounds()
            executeV3(calls, resourceBounds.l1Gas, resourceBounds.l2Gas)

to more intuitive:

            val resourceBounds = estimateFee.values.first().toResourceBounds()
            executeV3(calls, resourceBounds)

that doesn't require any knowledge of l1 or l2 gas to execute txs with auto estimated fees

Copy link
Collaborator Author

@franciszekjob franciszekjob Oct 24, 2024

Choose a reason for hiding this comment

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

Definitely, makes sense. Refactored.

I just don't fully like the fact that now the param of type ResourceBoundsMapping is named resourceBounds 😆 , but looks like it's because of this, so we can leave it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just don't fully like the fact that now the param of type ResourceBoundsMapping is named resourceBounds 😆 , but looks like it's because of this, so we can leave it.

That was the idea, but also the fact that resourceBoundsMapping doesn't seem like a good argument name from UX point of view.

I think such naming convention also makes sense in this particular case:

  • ResourceBounds - l1ResourceBounds, l2ResourceBounds
  • ResourceBoundsMapping - resourceBounds

@@ -65,7 +65,7 @@ data class FunctionInvocation(
val messages: List<OrderedMessageL2ToL1>,

@SerialName("execution_resources")
val computationResources: ComputationResources,
val computationResources: ExecutionResources,
Copy link
Contributor

Choose a reason for hiding this comment

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

In the spec this still points to non-existent COMPUTATION_RESOURCES:
https://github.com/starkware-libs/starknet-specs/blob/647caa00c0223e1daab1b2f3acc4e613ba2138aa/api/starknet_trace_api_openrpc.json#L448

Probably something we should clarify

Copy link
Collaborator Author

@franciszekjob franciszekjob Oct 24, 2024

Choose a reason for hiding this comment

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

Here it points correctly (to EXECUTION_RESOURCES)

Copy link
Contributor

Choose a reason for hiding this comment

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

The name's also changed to Execution resources there.

I wonder if we should keep try to keep the old name to avoid breaking change, or do it earlier than later, since the spec is expected to introduce some breaking changes anyways?

Comment on lines +94 to +95
@SerialName("block_id")
val blockId: BlockId,
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

@franciszekjob franciszekjob Oct 24, 2024

Choose a reason for hiding this comment

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

It is, please look here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, was just checking out the changes from the tag, not branch 😅

@franciszekjob
Copy link
Collaborator Author

franciszekjob commented Oct 24, 2024

Please check out spec on v0.8.0 branch while reviewing. Unfortunately, there were some changes so the listed PRs may not be fully up to date.

Copy link
Contributor

@DelevoXDG DelevoXDG left a comment

Choose a reason for hiding this comment

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

I like where this is going 👍🏻 Waiting for the tests

Comment on lines +544 to +547
resourceBounds = ResourceBoundsMapping(
ResourceBounds.ZERO,
ResourceBounds.ZERO,
),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:

Should we introduce ResourceBoundsMapping.ZERO? 🤔

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 was also thinking about this, should be handy 👍 .

Comment on lines +94 to +95
@SerialName("block_id")
val blockId: BlockId,
Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, was just checking out the changes from the tag, not branch 😅

Comment on lines +217 to +221
@SerialName("node")
val node: MerkleNode,
) {
@Serializable
sealed interface MerkleNode
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure that works?

Seems to me like some custom serializer is needed for node 🤔

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.

Support JSON-RPC v0.8.0
2 participants