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

Refactor StarknetProvider and StarknetRequest #217

Merged
merged 18 commits into from
Sep 2, 2024

Conversation

franciszekjob
Copy link
Collaborator

@franciszekjob franciszekjob commented Aug 29, 2024

Describe your changes

This PR focuses on refactoring StarknetProvider, StarknetRequest and related structs/protocols for sending RPC calls.

Introduced changes:

  • Move logic responsible for building requests form StarknetProvider to StarknetRequest which is now just a container for a RPC call.
  • Remove send method from StarknetRequest.
  • Add send method to StarknetProvider responsible for sending single/multiple RPC calls.

Linked issues

Closes #215

Breaking changes

  • This issue contains breaking changes
  • Move methods responsible for building RPC calls from StarknetProvider to StarknetRequest.
  • Remove send method from StarknetRequest - use added send in StarknetProvider to send RPC calls
  • Add provider param in estimateFeeV1 and estimateFeeV3 in StarknetAccountProtocol; Update following methods in StarknetAccount

@@ -16,7 +16,7 @@ public class StarknetAccount: StarknetAccountProtocol {
public let chainId: StarknetChainId

private let signer: StarknetSignerProtocol
private let provider: StarknetProviderProtocol
public let provider: StarknetProviderProtocol
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: provider needs to be changed to public, because it's added in StarknetAccountProtocol here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm, I understand that it's used in the extensions, but I wonder if provider should be included in the account protocol, as it's making the protocol reliant on component that is more implementation-specific (rpc communication), therefore making the account protocol less abstract

Copy link
Collaborator Author

@franciszekjob franciszekjob Aug 30, 2024

Choose a reason for hiding this comment

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

as it's making the protocol reliant on component that is more implementation-specific (rpc communication), therefore making the account protocol less abstract

I agree 👍 Just one remark - not adding provider in StarknetAccountProtocol enforces to add extra provider param in estimateFeeV1 and estimateFeeV3. Hope this one is not too cumbersome while using these methods.

Wdyt guys @DelevoXDG @mluisbrown?

Choose a reason for hiding this comment

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

We would only need to pass in the provider in the estimate methods that don't take nonce param, right? That seems ok to me.

Choose a reason for hiding this comment

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

We would keep the provider property in StarknetAccount though, right? It doesn't need to be in the protocol but it needs to be a public property in StarknetAccount.

Copy link
Collaborator Author

@franciszekjob franciszekjob Aug 30, 2024

Choose a reason for hiding this comment

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

Ofc, we will keep it in StarknetAccount . I added provider param to estimateFeeV1/3 which don't have nonce param 👍 .

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't we just make the methods abstract and define them in StarknetAccount instead?

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: Deleting as it's not used anymore.

Copy link
Collaborator Author

@franciszekjob franciszekjob Aug 29, 2024

Choose a reason for hiding this comment

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

note: As discussed with Argent team - now we want RPC calls to be static methods.

@franciszekjob franciszekjob requested a review from THenry14 August 29, 2024 12:43
@kkawula kkawula requested review from kkawula and removed request for kkawula August 29, 2024 12:51
@franciszekjob franciszekjob requested a review from kkawula August 29, 2024 12:52
@THenry14
Copy link
Member

@DelevoXDG please also take a look

@THenry14 THenry14 requested a review from DelevoXDG August 30, 2024 10:47
Copy link

@mluisbrown mluisbrown left a comment

Choose a reason for hiding this comment

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

Please see my comment about having the static request methods in an extension of the generic StarknetRequest type, which makes using them more cumbersome than necessary.

Sources/Starknet/Network/StarknetRequest.swift Outdated Show resolved Hide resolved
Copy link

@mluisbrown mluisbrown left a comment

Choose a reason for hiding this comment

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

All looks great now. Integrated into Argent and also ran our tests against it and all good.
Nice job 👍

@franciszekjob franciszekjob merged commit 974f038 into main Sep 2, 2024
1 check passed
@franciszekjob franciszekjob deleted the feat/215-starknet-provider-refactor branch September 2, 2024 07:13
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.

Refactor StarknetProvider and StarknetRequest
4 participants