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 upgrade docs #731

Merged

Conversation

ericnordelo
Copy link
Member

Fixes #559

I used #567 as base because is easier to follow the organization and fix links among components like that. This PR will be smaller after merging that one.

ericnordelo and others added 30 commits August 23, 2023 14:27
…o/cairo-contracts into feat/update-account-docs-#560
Copy link
Collaborator

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

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

This looks about good to go! I just left a question

Comment on lines 81 to 89
=== Interface


[,javascript]
----
trait IUpgradeable {
fn upgrade(new_class_hash: ClassHash);
}
----
Copy link
Collaborator

Choose a reason for hiding this comment

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

Forgive me, I should have brought this up in my last review. Do you think it makes sense to display Interface before Usage? To me, having the definition before an implementation example is easier to follow for the reader. That's also sort of the organization with Introspection and Accounts (without the explicit Usage section). Conversely, the Access Control PR presents Interface after Usage as in here. I vote we normalize and I lean toward Interface before Usage. Wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's no much to say about the interface, and I don't particularly like just exposing it before the usage section. I refactored it to add a Note in the usage section with a link to the API Reference, let me know what you think.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There's no much to say about the interface

Haha that's fair. "Here's the interface with one function..."

I don't particularly like just exposing it before the usage section

In general or just in this particular case?

Copy link
Member Author

Choose a reason for hiding this comment

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

In general or just in this particular case?

In this case

Copy link
Collaborator

Choose a reason for hiding this comment

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

I refactored it to add a Note in the usage section with a link to the API Reference, let me know what you think.

I honestly like the idea of displaying the interface in the main page, but I do think think removing the interface looks better here

Copy link
Collaborator

Choose a reason for hiding this comment

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

And when I "think think", you know it's the truth :)

Copy link
Contributor

@martriay martriay Sep 27, 2023

Choose a reason for hiding this comment

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

I think we should add the interface section and expand on it, but we can do it once we write a SNIP for it. I do think it's important to document an interface, it doesn't matter if it has 1 or 10 methods. Also remember this interface is meant for interoperability and its usage is different the module's. They're almost two different things, this module is not a preset like ERC20 where the module and the interface are very closely related (one implements the other).

some contracts might use the upgrades module but not implement the interface nor expose the functionality in any way.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree it might be better to do it after having a standard.

docs/modules/ROOT/pages/upgrades.adoc Show resolved Hide resolved
Copy link
Collaborator

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

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

Left a tiny suggestion in the new note. Otherwise, LGTM! Great work

docs/modules/ROOT/pages/upgrades.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/upgrades.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/upgrades.adoc Show resolved Hide resolved
Comment on lines 92 to 93
can't be implemented. Instead, a proxy contract must be implemented with each specific function that is going to be redirected.
This can still be useful for example with upgrading the logic of some functions.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
can't be implemented. Instead, a proxy contract must be implemented with each specific function that is going to be redirected.
This can still be useful for example with upgrading the logic of some functions.
can't be implemented. Instead, a limited proxy contract can implement specific functions that forward their execution to another contract class.
This can still be useful for example to upgrade the logic of some functions.

i think this reads better, but still not convinced about documenting a half baked proxy design

Copy link
Member Author

Choose a reason for hiding this comment

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

I wouldn't say a limited proxy, different proxy patterns doesn't make them limited, I think having a fallback makes proxies easier to implement, but as with the examples I shared, we can implement proxies without it, I wouldn't call them limited proxies. We can certainly define an SRC for a fallback at application level for proxies, why would we call that limited?

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO we don't have a half baked proxy design, there are multiple potential proxy designs, and we don't have one, but we have the ability to implement them, proxies can be implemented in Starknet (different patterns).

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 having a fallback makes proxies easier to implement, but as with the examples I shared, we can implement proxies without it

agree

I wouldn't call them limited proxies

I would since there's strong limitations as not being able to support arbitrary function forwarding (fallback function).

IMO we don't have a half baked proxy design, there are multiple potential proxy designs, and we don't have one, but we have the ability to implement them, proxies can be implemented in Starknet (different patterns).

exactly because we don't have one is that I called the proxy described as "half baked". It's a (very valid) assumption of how a proxy would look like but it's briefly explained, without a clear use case, and requires to be more thoroughly thought before being described/suggested in our official docs, as they're seen as a place to learn about best practices.

@ericnordelo ericnordelo merged commit fe695e4 into OpenZeppelin:cairo-2 Sep 29, 2023
1 check passed
@ericnordelo ericnordelo deleted the feat/update-upgrade-docs-#559 branch September 29, 2023 15:31
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.

update upgrades docs
3 participants