Update security docs #794

ericnordelo · 2023-10-19T10:18:30Z

Fixes #565

PR Checklist

Tests
Tried the feature on a public network
Documentation

andrew-fleming

Looking good, sir! I left some comments, questions, and suggestions

docs/modules/ROOT/pages/security.adoc

src/security.cairo

src/tests/security/test_pausable.cairo

andrew-fleming · 2023-10-19T20:30:00Z

src/security/pausable.cairo

-        fn _pause(ref self: ComponentState<TContractState>) {
+        fn pause(ref self: ComponentState<TContractState>) {


I assume this is for consistency, right? In the past, we followed the convention that underscored methods signified that it was potentially unsafe e.g. _mint. We should probably prioritize defining the new standard moving forward so that cases like this are clear. I don't love the idea of changing this without said standard

Fair point. Agree is better wait for that discussion to happen.

docs/modules/ROOT/pages/api/security.adoc

andrew-fleming · 2023-10-19T21:04:47Z

src/security/initializable.cairo

+trait ExternalTrait<TState> {
+    fn is_initialized(self: @TState) -> bool;
+}


No strong opinions on including this trait. I suppose there may be cases where this is useful

The name IPausable sounds a bit misleading IMO (for an interface with just the is_paused method), I think for this case is better to use a trait (like PublicKeyTrait in account), I selected the name ExternalTrait because no better name occurred to me for just is_paused. We should document this convention/pattern if we go along with it. What do you think @martriay ?

The name IPausable sounds a bit misleading IMO (for an interface with just the is_paused method)

Good point. Here, it'd be IInitializable, but the point is the same. I can see it being misleading with just the getter

We should document this convention/pattern if we go along with it.

I just wrote this recently in our style guide, which is something I recall we discussed maybe during a call. Let me know if you think it doesn't apply here

The name IPausable sounds a bit misleading IMO (for an interface with just the is_paused method)

I kindof see what you mean, but on the other hand "pausable" doesn't really mean I can pause it, but rather that it can be paused, so the getter is not really confusing to me. btw Contracts for Solidity's version has public pause/unpause methods

Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>

…lo/cairo-contracts into feat/update-security-docs-#565

andrew-fleming

I left one last suggestion. Assuming we all like ExternalTrait in these instances, LGTM!

docs/modules/ROOT/pages/security.adoc

Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>

…lo/cairo-contracts into feat/update-security-docs-#565

…eat/update-security-docs-#565

andrew-fleming

LGTM!

martriay

Docs look really good! Left some comments.

src/tests/security/test_pausable.cairo

martriay · 2023-11-10T06:15:26Z

src/tests/security/test_initializable.cairo

@@ -1,4 +1,4 @@
-use openzeppelin::security::initializable::InitializableComponent::InternalImpl;
+use openzeppelin::security::InitializableComponent::{InitializableImpl, InternalImpl};


why was this needed?

To call is_initialized later

but why wasn't it failing?

src/tests/mocks/initializable_mock.cairo

martriay · 2023-11-10T06:19:19Z

src/security/initializable.cairo

+trait ExternalTrait<TState> {
+    fn is_initialized(self: @TState) -> bool;
+}


We should document this convention/pattern if we go along with it.

I just wrote this recently in our style guide, which is something I recall we discussed maybe during a call. Let me know if you think it doesn't apply here

The name IPausable sounds a bit misleading IMO (for an interface with just the is_paused method)

I kindof see what you mean, but on the other hand "pausable" doesn't really mean I can pause it, but rather that it can be paused, so the getter is not really confusing to me. btw Contracts for Solidity's version has public pause/unpause methods

src/security/pausable.cairo

docs/modules/ROOT/pages/api/security.adoc

Co-authored-by: Martín Triay <martriay@gmail.com>

…lo/cairo-contracts into feat/update-security-docs-#565

Co-authored-by: Martín Triay <martriay@gmail.com>

…lo/cairo-contracts into feat/update-security-docs-#565

src/tests/mocks/initializable_mock.cairo

ericnordelo added 6 commits January 21, 2023 00:19

fix: link (OpenZeppelin#545)

331844d

Merge branch 'main' of github.com:OpenZeppelin/cairo-contracts

2e7758e

Merge branch 'main' of github.com:OpenZeppelin/cairo-contracts

77cca2d

Merge branch 'main' of github.com:OpenZeppelin/cairo-contracts

8a19908

feat: add API Reference to docs

0dabc59

feat: update docs page

9850dd5

ericnordelo requested review from andrew-fleming and martriay October 19, 2023 10:19

feat: shorten imports

2e28266

andrew-fleming reviewed Oct 19, 2023

View reviewed changes

ericnordelo and others added 3 commits October 20, 2023 11:00

Update docs/modules/ROOT/pages/api/security.adoc

cb6fd01

Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>

feat: apply review updates

05d7bef

Merge branch 'feat/update-security-docs-#565' of github.com:ericnorde…

5207a65

…lo/cairo-contracts into feat/update-security-docs-#565

ericnordelo requested a review from andrew-fleming October 20, 2023 09:05

andrew-fleming approved these changes Oct 20, 2023

View reviewed changes

docs/modules/ROOT/pages/security.adoc Outdated Show resolved Hide resolved

ericnordelo mentioned this pull request Oct 21, 2023

Migrate Upgrades to component #792

Merged

3 tasks

ericnordelo and others added 3 commits October 23, 2023 12:25

Update docs/modules/ROOT/pages/security.adoc

d2876e9

Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>

docs: flatten events in examples

643e97a

Merge branch 'feat/update-security-docs-#565' of github.com:ericnorde…

a624d29

…lo/cairo-contracts into feat/update-security-docs-#565

andrew-fleming mentioned this pull request Oct 25, 2023

Migrate Account to component #783

Merged

3 tasks

ericnordelo added 2 commits November 2, 2023 14:57

Merge branch 'main' of github.com:OpenZeppelin/cairo-contracts into f…

3eb8cb8

…eat/update-security-docs-#565

docs: update headers

b51d3bf

ericnordelo requested a review from andrew-fleming November 2, 2023 14:08

andrew-fleming approved these changes Nov 2, 2023

View reviewed changes

martriay reviewed Nov 10, 2023

View reviewed changes

ericnordelo and others added 5 commits November 10, 2023 13:06

feat: remove external trait

710ed6f

Update docs/modules/ROOT/pages/security.adoc

1f39593

Co-authored-by: Martín Triay <martriay@gmail.com>

Update docs/modules/ROOT/pages/api/security.adoc

2a6afe2

Co-authored-by: Martín Triay <martriay@gmail.com>

Update docs/modules/ROOT/pages/api/security.adoc

4a07f9f

Co-authored-by: Martín Triay <martriay@gmail.com>

Update docs/modules/ROOT/pages/api/security.adoc

be95dc7

Co-authored-by: Martín Triay <martriay@gmail.com>

ericnordelo and others added 9 commits November 10, 2023 13:14

feat: apply review updates

2b8354a

Merge branch 'feat/update-security-docs-#565' of github.com:ericnorde…

48fb908

…lo/cairo-contracts into feat/update-security-docs-#565

Update docs/modules/ROOT/pages/security.adoc

38f1b22

Co-authored-by: Martín Triay <martriay@gmail.com>

Update docs/modules/ROOT/pages/api/security.adoc

2c6dc8e

Co-authored-by: Martín Triay <martriay@gmail.com>

Update docs/modules/ROOT/pages/api/security.adoc

4a2c109

Co-authored-by: Martín Triay <martriay@gmail.com>

Update docs/modules/ROOT/pages/api/security.adoc

44aceec

Co-authored-by: Martín Triay <martriay@gmail.com>

Update docs/modules/ROOT/pages/api/security.adoc

25b76f2

Co-authored-by: Martín Triay <martriay@gmail.com>

feat: apply review updates

45e860d

Merge branch 'feat/update-security-docs-#565' of github.com:ericnorde…

3acf822

…lo/cairo-contracts into feat/update-security-docs-#565

ericnordelo requested a review from martriay November 10, 2023 12:21

martriay approved these changes Nov 10, 2023

View reviewed changes

src/tests/mocks/initializable_mock.cairo Show resolved Hide resolved

martriay merged commit 68e9f37 into OpenZeppelin:main Nov 10, 2023
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update security docs #794

Update security docs #794

ericnordelo commented Oct 19, 2023

andrew-fleming left a comment

andrew-fleming Oct 19, 2023

ericnordelo Oct 20, 2023

andrew-fleming Oct 19, 2023

ericnordelo Oct 20, 2023

andrew-fleming Oct 20, 2023

martriay Nov 10, 2023

andrew-fleming left a comment

andrew-fleming left a comment

martriay left a comment

martriay Nov 10, 2023

ericnordelo Nov 10, 2023

martriay Nov 10, 2023

martriay Nov 10, 2023

		fn _pause(ref self: ComponentState<TContractState>) {
		fn pause(ref self: ComponentState<TContractState>) {

		@@ -1,4 +1,4 @@
		use openzeppelin::security::initializable::InitializableComponent::InternalImpl;
		use openzeppelin::security::InitializableComponent::{InitializableImpl, InternalImpl};

Update security docs #794

Update security docs #794

Conversation

ericnordelo commented Oct 19, 2023

PR Checklist

andrew-fleming left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrew-fleming left a comment

Choose a reason for hiding this comment

andrew-fleming left a comment

Choose a reason for hiding this comment

martriay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment