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.

Sign up for GitHub

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

Jump to bottom

CapabilityFactory.Manager.addFactory function overwrites existing types #73

Closed
Tracked by #59
sisyphusSmiling opened this issue Jun 12, 2023 · 0 comments · Fixed by #84
Closed
Tracked by #59

CapabilityFactory.Manager.addFactory function overwrites existing types #73

sisyphusSmiling opened this issue Jun 12, 2023 · 0 comments · Fixed by #84
Assignees
@sisyphusSmiling
Labels
enhancement New feature or request

Comments

@sisyphusSmiling
Copy link
Collaborator

Description

In contracts/CapabilityFactory.cdc:17, the function addFactory() doesn’t check if the type that is being added already exists or not. If the type to be added already exists, it may be overwritten by mistake.

Recommendation

We recommend implementing a updateFactory() function which allows updating existing factory types and modifying the addFactory() function only to accept new types. Alternatively, we recommend documenting the intended behavior of the function to educate developers.
@sisyphusSmiling sisyphusSmiling added the enhancement New feature or request label Jun 12, 2023
@sisyphusSmiling sisyphusSmiling mentioned this issue Jun 12, 2023
Closed
15 tasks
@sisyphusSmiling sisyphusSmiling self-assigned this Jun 14, 2023
@sisyphusSmiling sisyphusSmiling mentioned this issue Jun 15, 2023
Merged
sisyphusSmiling added a commit that referenced this issue Jun 20, 2023
@sisyphusSmiling
Update CapabilityFactory Manager (#84)
14a5f8a 
Closes: #73 
Coverage: 75.6%

Added pre condition to `Manager.addFactory()` to prevent overwriting
existing factories. Also added `Manager.updateFactory()` which
overwrites Factories on given Type, and adds if it didn't already exist.
The thought here is that the behavior is more explicit and provides a
safer method to add new Factory types depending on developer preference,
but open to feedback on this behavior.

Also added comments to CapabilityFactory contract.

Last note, I added a delimiter `'_'` to derived canonical paths between
the prefix and deployment Address. I thought this would make it easier
to split the strings should the need arise.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sisyphusSmiling sisyphusSmiling

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Update CapabilityFactory Manager onflow/hybrid-custody
1 participant
@sisyphusSmiling