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

UpgradeSoulGasToken.s.sol: get impl correctly #173

Merged
merged 3 commits into from
Jan 24, 2025
Merged

Conversation

blockchaindevsh
Copy link
Collaborator

@blockchaindevsh blockchaindevsh commented Jan 23, 2025

The Proxy.implementation() method can only be called from ProxyAdmin, otherwise will revert since there's no such method in the implementation contract.

UPDATE

This PR also ensures that sgt owner is not changed during the upgrade with 1f2f2c9.

@dajuguan
Copy link

How can we test it before deploying? Should we deploy it on the devnet first?

@blockchaindevsh
Copy link
Collaborator Author

How can we test it before deploying? Should we deploy it on the devnet first?

There's already a checkOutput() method, no transaction will be sent if the check fails.

@dajuguan
Copy link

Is it feasible to list the pre- and post-condition checks in the contracts to be deployed, so it will be easier for reviewers to understand what's going on?

@blockchaindevsh
Copy link
Collaborator Author

Is it feasible to list the pre- and post-condition checks in the contracts to be deployed, so it will be easier for reviewers to understand what's going on?

Don't quite understand, could you provide an example?

@dajuguan
Copy link

In this PR:
Precondition:

  • only proxyAdmin can call

Postcondition:

  • soulGasToken.name() == "SoulQKC"
  • soulGasToken.symbol() == "SoulQKC"
  • soulGasToken.owner == preOwner

@blockchaindevsh
Copy link
Collaborator Author

In this PR: Precondition:

  • only proxyAdmin can call

Postcondition:

  • soulGasToken.name() == "SoulQKC"
  • soulGasToken.symbol() == "SoulQKC"
  • soulGasToken.owner == preOwner

I can understand the Postcondition part, what do you want to do with the Precondition part?

@dajuguan
Copy link

For now, the only thing about Precondition I can think of is related to permissions. It's just an example

@dajuguan
Copy link

Nice!

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.

3 participants