Upgrade contract & fix deployment script #447
Conversation
| /// this function can safely be implemented as: | ||
| /// ```ignore | ||
| /// Ok(Response::default()) | ||
| /// ``` |
There was a problem hiding this comment.
(not sure if there's a difference between ::new and ::default, but wormhole is using ::default)
| "type": "module", | ||
| "scripts": { | ||
| "test": "echo \"Error: no test specified\" && exit 1" | ||
| "deploy-pyth": "node deploy-pyth-bridge.js" |
There was a problem hiding this comment.
the README claims this command exists, but it doesn't for whatever reason.
| Err(PythContractError::InvalidGovernancePayload)? | ||
| } | ||
|
|
||
| // Hack: We're using the first 8 bytes of the address field as the new code id. |
There was a problem hiding this comment.
Sorry for the inconsistency here.. for this one aptos uses 32 bytes and Eth uses 20 bytes. I think it is fine to define the governance instruction to use only 8 bytes instead of getting a prefix of a longer address.
Although not used anywhere, you will need to define it here too (for scripts/tools that manage deployments):
https://github.com/pyth-network/pyth-crosschain/blob/main/third_party/pyth/xc-governance-sdk-js/src/instructions.ts#L103-L121
| }, | ||
| governance_source_index: 0, | ||
| governance_sequence_number: 0, | ||
| chain_id: 18, |
There was a problem hiding this comment.
not for this pr, can we query this from wormhole contract instead of manually specifying it?
There was a problem hiding this comment.
I looked into this as part of the cleanup PR before. Unfortunately wormhole doesn't expose a query for it. We could read it out of their config struct, but that felt a little hacky. wdyt?
There was a problem hiding this comment.
or we could add support to it to the wormhole contract?
There was a problem hiding this comment.
ok never mind then 😅 second one is good but it takes time for wh to merge and then deploy it. let's stick to this one 😅
ali-behjati
left a comment
ali-behjati
left a comment
There was a problem hiding this comment.
I left some comments about the upgrade payload (and adding it to the js sdk). I do not fully know general cosmos and terra differences and I have a question before approving this one: Don't we need to change the admin of the contract to itself? If so, can we add that to the deploy script? iirc we cannot know the contract address before instantiation and it should be a separate message.
Add the code for upgrading the contract and patch up the deployment script so it works on the new Terra network.
I haven't tested the upgrade process yet, as it's hard to do with the current deployment tools. I'll do some more work on the deployment process and then circle back to testing upgrades.