Skip to content
This repository has been archived by the owner on Dec 1, 2023. It is now read-only.

Add function to update registered deployment ABI #211

Merged
merged 15 commits into from
Oct 3, 2022
Merged

Add function to update registered deployment ABI #211

merged 15 commits into from
Oct 3, 2022

Conversation

ericglau
Copy link
Member

@ericglau ericglau commented Sep 23, 2022 •
edited
Loading

Fixes #204

The update_abi function takes an identifier (address or alias) as input, and updates the ABI while keeping the same address and alias.

@ericglau ericglau mentioned this pull request Sep 27, 2022
Closed
@ericglau ericglau changed the title Add function to update registered deployment Add function to update registered deployment ABI Sep 28, 2022
@ericglau
Copy link
Member Author

ericglau commented Sep 28, 2022 •
edited
Loading

Should this function be named update_abi for clarity? I don't think there is any need to update a deployment's alias.

@ericglau ericglau marked this pull request as draft September 28, 2022 21:14
@andrew-fleming
Copy link
Contributor

Should this function be named update_abi for clarity? I don't think there is any need to update a deployment's alias.

Yeah, update_abi works much better IMO; although, I can see updating an alias as a (separate) useful feature

@ericglau ericglau marked this pull request as ready for review September 29, 2022 18:05
andrew-fleming
andrew-fleming previously approved these changes Sep 29, 2022
View reviewed changes
Copy link
Contributor

@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.

Very nice! Looks good to me

ericnordelo
ericnordelo suggested changes Sep 29, 2022
View reviewed changes
Copy link
Member

@ericnordelo ericnordelo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice addition! I left just a comment for keeping consistency with allowing multiple aliases.

src/nile/deployments.py Outdated Show resolved Hide resolved
ericnordelo
ericnordelo reviewed Oct 3, 2022
View reviewed changes
Copy link
Member

@ericnordelo ericnordelo left a comment

Choose a reason for hiding this comment

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

Looking good! Just added a suggestion to keep code consistency and I think is good to go.

src/nile/deployments.py Outdated Show resolved Hide resolved
ericnordelo
ericnordelo approved these changes Oct 3, 2022
View reviewed changes
Copy link
Member

@ericnordelo ericnordelo left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@ericglau ericglau merged commit 4d1ebf4 into OpenZeppelin:main Oct 3, 2022
@ericglau ericglau deleted the 204 branch October 3, 2022 21:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ericnordelo ericnordelo ericnordelo approved these changes

@martriay martriay Awaiting requested review from martriay

@andrew-fleming andrew-fleming Awaiting requested review from andrew-fleming

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add function to update a deployment in the deployments file
3 participants
@ericglau @andrew-fleming @ericnordelo