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

Adapt Account to OpenZeppelin 0.1.0 and Argent 0.2.1 #108

Merged
merged 14 commits into from
May 26, 2022
Merged

Conversation

FabijanC
Copy link
Collaborator

@FabijanC FabijanC commented May 13, 2022

Usage related changes

Development related changes

  • Read account artifacts from the plugin repo (not from the example repo)
  • Introduced rawOutput manipulation because the Argent implementation returns raw output from its __execute__
  • Rename handleMultiCall -> handleMultiInteract and make it a common Account method.

Checklist:

  • I have formatted the code
  • I have performed a self-review of the code
  • I have rebased to the base branch
  • I have documented the changes
  • I didn't have to update the tests
  • I didn't have to create a PR to the plugin branch of starknet-hardhat-example.

@FabijanC FabijanC requested a review from badurinantun May 13, 2022 16:47
@FabijanC FabijanC changed the title Update to Adapt to OpenZeppelin 0.1.0 and Argent 0.2.1 May 16, 2022
@FabijanC FabijanC changed the title Adapt to OpenZeppelin 0.1.0 and Argent 0.2.1 Adapt Account to OpenZeppelin 0.1.0 and Argent 0.2.1 May 16, 2022
@FabijanC FabijanC requested review from badurinantun and removed request for badurinantun May 16, 2022 07:27
@FabijanC FabijanC marked this pull request as draft May 16, 2022 14:45
@FabijanC FabijanC marked this pull request as ready for review May 17, 2022 08:07
// Full path to where the artifacts will be saved
const artifactsTargetPath = path.join(
baseArtifactsPath,
ACCOUNT_ARTIFACTS_VERSION,
accountType,
artifactsVersion,
artifactsBase
);

const jsonArtifact = artifactsName + ".json";
const abiArtifact = artifactsName + ABI_SUFFIX;

const artifactLocationUrl = GITHUB_ACCOUNT_ARTIFACTS_URL.concat(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we using Github repo for this? Why don't we just load the artifacts from the directory?

Copy link
Collaborator Author

@FabijanC FabijanC May 18, 2022

Choose a reason for hiding this comment

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

Changed downloading from github to copying from the plugin: 188a6e2
Set to use unminifed artifacts with debug info: c78632c

@FabijanC FabijanC requested a review from badurinantun May 18, 2022 13:43
const { nonce } = await this.starknetContract.call("get_nonce");
return nonce;
}

protected hasRawOutput(): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this a function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you're asking why it isn't a property, it doesn't have to be, it's just following the same paradigm as getExecutionFunctionName, which could also be a property set through constructor.

});
});
}
const baseArtifactsPath = path.join(hre.config.paths.starknetArtifacts, ACCOUNT_ARTIFACTS_DIR);

// Full path to where the artifacts will be saved
const artifactsTargetPath = path.join(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are artifacs being saved to a target directory?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Accounts are also contracts. So for the plugin to be able to interact with them as contracts, their artifacts need to be in the artifacts directory. Internally, doing account.call(...) does something like account.starknetContract.call(...)

@FabijanC FabijanC merged commit a8eb495 into master May 26, 2022
@FabijanC FabijanC deleted the account-update branch May 26, 2022 07:32
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