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

Update Account to namespace, fix docs #297

Merged
merged 23 commits into from May 6, 2022

Conversation

andrew-fleming
Copy link
Collaborator

This PR updates the Account library and contract to utilize namespace. This PR also updates the Account documentation. Since users requested better documentation, this not only updates the docs but provides a more thorough explanation regarding AccountCallArray and the flow of transactions.

This resolves #258 and resolves #271.

Copy link
Contributor

@martriay martriay left a comment

Choose a reason for hiding this comment

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

Good improvements! Please fix the merge conflicts.

docs/Account.md Outdated Show resolved Hide resolved
docs/Account.md Outdated

2. The `__execute__` method creates an empty array which is passed with the `AccountCallArray` and calldata into the `_from_call_array_to_call` method. This method iterates through the `AccountCallArray` messages and converts each message into a `Call`.

3. A new empty array is created and passed with the list of calls (populated from `from_call_array_to_call`) to `_execute_list`. This method iterates and executes a contract call for each `Call` and pushes each result into the array. Once all contract calls are finished, the array holding each contract call response is returned.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is just an implementation detail, it's the interface what we want to document, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correct. I broke this down for myself so I could better explain/understand the process. A blog post or something might be better suited to document the implementation details. Good call

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's a bit confusing to go that deep into this kind details, e.g. we could say "An array of Calls is passed to _execute_list" instead -- no need to explain that a new array is allocated, iterated, etc. I'd remove this section alltogether.

Copy link
Contributor

@martriay martriay May 6, 2022

Choose a reason for hiding this comment

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

sorry by "section" I only meant point 3! Quick recap: I do believe this explanation is useful for users to understand why do we have an intermediate representation of the Multicall, so interfaces + structures should be described. How the functions work internally to handle these interfaces + structs is what I think is not necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hahaha whoops :) my mistake

@martriay martriay merged commit 75ed19a into OpenZeppelin:main May 6, 2022
@andrew-fleming andrew-fleming deleted the namespace-account branch May 6, 2022 15:08
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.

No MultiCall in code Explain AccountCallArray struct in Account docs
2 participants