-
Notifications
You must be signed in to change notification settings - Fork 279
feat(lazer/sui): state and initializer #2972
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
Conversation
Co-Authored-By: Tejas Badadare <[email protected]>
…rosschain into tb/lazer/sui-storage
Co-Authored-By: Tejas Badadare <[email protected]>
…rosschain into tb/lazer/sui-storage
…and tests; verified with Sui CLI 1.53.2 Co-Authored-By: Tejas Badadare <[email protected]>
…rosschain into tb/lazer/sui-storage
… CLI 1.53.2 Co-Authored-By: Tejas Badadare <[email protected]>
…d gate updates with admin cap Co-Authored-By: Tejas Badadare <[email protected]>
…rosschain into tb/lazer/sui-storage
…dminCapability Co-Authored-By: Tejas Badadare <[email protected]>
…er via AdminCapability" This reverts commit 187f0e9.
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…_lazer::init; share State; add OTW doc comments Co-Authored-By: Tejas Badadare <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, seems straightforward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments. I'll approve after you help me understand it slightly better and this is complicated.
Re: admin cap ownership. The DAO should control everything in the end and in the Pyth Core contract, by some tricks we let the contract own it's own admin cap.
|
||
/// Initializes the module. Called at publish time. | ||
/// Creates and shares the singular State object. | ||
/// AdminCap is created and transferred in admin::init via a One-Time Witness. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm it's not here right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it's happening in the admin module's init, but it should happen within the same publish PTB
/// Only the AdminCap owner can update the trusted signers. | ||
fun init(otw: ADMIN, ctx: &mut TxContext) { | ||
assert!(types::is_one_time_witness(&otw), 1); | ||
let cap = AdminCap { id: object::new(ctx) }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AdminCap can be created in other places by mistake no? If you want the admin cap to be only made by one, you need to enforce it via a type (like the cap retaining the witness, or having a phantom data or something).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, the existing pyth contract also can't guard against new logic adding the ability to edit state. Any friend of state can call assert_latest_only and obtain a LatestOnly cap that lets them edit state. Currently governance and the "create price feeds" codepaths call it but others could be introduced in the future.
If the AdminCap retains the witness, doesn't the vuln just move to the witness? i.e. some codepath can be introduced in the future that can instantiate the witness, allowing instantiation of the AdminCap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm you can't get the witness out of AdminCap without deconstructing it, right?
Regardless, I was thinking about the problem you are trying to solve (not being able to instantiate AdminCap twice) and I'm not sure we really need to solve it. We generally trust ourselves and not the others and in the future might have good reasons to have two or divide it in two capabilities.
The usage of witness also seems to be where you want external people to behave in a certain way and not us. (like someone is creating a Coin and we want to impose some certain characteristic there)
This comment was marked as outdated.
This comment was marked as outdated.
@ali-behjati Okay today's convo on the future plan help elucidate this for me. How the Pyth contract works is:
For this iteration of the Lazer contract:
Thus, I currently can't have the contract store its own UpgradeCap since then we'd have no way of upgrading it after deploying. The implementation in this PR basically gives UpgradeCap and AdminCap to the deployment tx sender, giving it full control. |
It's practically impossible. The DAO lives on Solana, it can't have a non-Solana account. We can setup a multi-sig or something on SUI and ask the Pythian council to own to form a lose connection to the DAO but it's much more work. To be an official Pyth contract, we need the governance to be there and that can't be done without using a bridge. That being said, the two governance methods you need are updating the signers and upgrade and shouldn't be difficult. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm approving this to unblock you, we'll likely need to revisit it again in the future.
Sounds good. Once we add the governance messages i'll add the wormhole governance support. |
Summary
Add a contract state module. The singleton
State
consists of the current set of trusted signers.state::update_trusted_signer
can be used to upsert trusted signer infoAdd initializers to
pyth_lazer
andadmin
modules. Initializers are called automatically once and only once at module publish time.admin::init
AdminCap
and transfers ownership to the deployer. We guarantee this can only be called once by using the One-Time Witness pattern. Althoughinit
will only be called once, it's possible for contract upgrades to introduce bugs that allow minting extra AdminCaps. The OTW pattern prevents this from happening, as the OTW struct will be consumed and dropped by the first init.state::update_trusted_signer
. This is done by specifying&AdminCap
as the first parameter. The type system guarantees that only the admin cap owner can obtain a reference (borrow) to it, ensuring only the admin is capable of calling the function. By naming the parameter_
, the cap reference is immediately consumed.pyth_lazer::init
state:update_trusted_signer
function.Next up:
How has this been tested?