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

feat: 4337 function #2

Merged
merged 10 commits into from
Dec 15, 2023
Merged

feat: 4337 function #2

merged 10 commits into from
Dec 15, 2023

Conversation

sanyu1225
Copy link
Collaborator

  • sendUserOperation
  • estimateUserOperationGas
  • getUserOperationByHash
  • getUserOperationReceipt
  • supportedEntryPoints
  • generateUserOpHash

@sanyu1225 sanyu1225 requested a review from jdevcs October 23, 2023 08:06
"version": "0.1.0",
"lockfileVersion": 2,
"requires": true,
"packages": {
Copy link
Contributor

Choose a reason for hiding this comment

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

There are both package-lock and yarn-lock files, you might be using npm and yarn both, I'll recommend to use only one package manager ( in this case yarn ).

});

// it("should call rpcMethods.estimateUserOperationGas with expected parameters", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

you will also need to add some E2E tests.

@jdevcs
Copy link
Contributor

jdevcs commented Nov 2, 2023

@sanyu1225 are you still working in this PR as its in draft or its ready ?

@sanyu1225
Copy link
Collaborator Author

sanyu1225 commented Nov 2, 2023

@sanyu1225 are you still working in this PR as its in draft or its ready ?

@jdevcs I will add some E2E tests, thx.

@sanyu1225 sanyu1225 marked this pull request as ready for review November 8, 2023 11:15
@sanyu1225
Copy link
Collaborator Author

@jdevcs I've finished adding the E2E tests and have reviewed everything. The PR is now updated and ready for review. Let me know if there are any changes needed. Thanks!

package.json Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
Copy link

@Muhammad-Altabba Muhammad-Altabba left a comment

Choose a reason for hiding this comment

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

Great work!
I have a small suggestion. In addition to Junaid comments.

Comment on lines 1 to 7
import { UserOperation } from "../type";

export type ValidInputTypes = Uint8Array | bigint | string | number | boolean;

export const isHexStrict = (hex: ValidInputTypes): boolean =>
typeof hex === "string" && /^((-)?0x[0-9a-f]+|(0x))$/i.test(hex);

Choose a reason for hiding this comment

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

You may use isHexStrict from web3-utils, if you like:

Suggested change
import { UserOperation } from "../type";
export type ValidInputTypes = Uint8Array | bigint | string | number | boolean;
export const isHexStrict = (hex: ValidInputTypes): boolean =>
typeof hex === "string" && /^((-)?0x[0-9a-f]+|(0x))$/i.test(hex);
import { isHexStrict } from "web3-utils";
import { UserOperation } from "../type";
export type ValidInputTypes = Uint8Array | bigint | string | number | boolean;

Copy link
Collaborator Author

@sanyu1225 sanyu1225 Nov 25, 2023

Choose a reason for hiding this comment

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

Thx, I noticed that the isHexStrict function from web3-utils was marked as deprecated in the documentation. So, I've switched to using the equivalent function from web3-validatorinstead. d450e29

@jdevcs
Copy link
Contributor

jdevcs commented Dec 11, 2023

@sanyu1225 some of CI actions are not passing.

@sanyu1225
Copy link
Collaborator Author

@sanyu1225 some of CI actions are not passing.

Hi, @jdevcs

The CI black box tests are failing because our latest version isn't published on npm yet. I suggest we merge the current changes and release a new version to fix this. Once updated, we can re-run the CI tests.

Let me know if you're okay with this plan!

Thanks!

@jdevcs
Copy link
Contributor

jdevcs commented Dec 15, 2023

@sanyu1225 some of CI actions are not passing.

Hi, @jdevcs

The CI black box tests are failing because our latest version isn't published on npm yet. I suggest we merge the current changes and release a new version to fix this. Once updated, we can re-run the CI tests.

Let me know if you're okay with this plan!

Thanks!

Hi, I'll suggest to create separate github action for build as compared to tests ( including black box ) . If you want you may do it in another PR?

@sanyu1225
Copy link
Collaborator Author

@sanyu1225 some of CI actions are not passing.

Hi, @jdevcs
The CI black box tests are failing because our latest version isn't published on npm yet. I suggest we merge the current changes and release a new version to fix this. Once updated, we can re-run the CI tests.
Let me know if you're okay with this plan!
Thanks!

Hi, I'll suggest to create separate github action for build as compared to tests ( including black box ) . If you want you may do it in another PR?

Sure, thx

@sanyu1225 sanyu1225 merged commit 46df21b into master Dec 15, 2023
4 checks passed
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.

3 participants