Conversation
|
👋 FelixFan1992, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
| } | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
currently, we can use a hack:
[published.testnet]
chain-id = "4c78adac"
published-at = "0x0000000000000000000000000000000000000000000000000000000000000000"
original-id = "0x0000000000000000000000000000000000000000000000000000000000000000"
version = 1
but this is not ideal. waiting for Sui team to release a new approach
|
Seems like the primary remaining item in this PR is the insufficient gas error. Likely due to account selection in the CLI. |
|
@FelixFan1992 Currently all tests should pass with the exception of the upgrades via MCMS. The issue with MCMS upgrades is that we are still using the CLI the build the compiled package.
When using the That should be the last missing piece. The summary is that we are now creating the |
|
Was able to get TokenPool upgrades via MCMS working. The remaining errors are the same in most test suites and are likely related to the setup of the test itself. |
|
Note: There is one test in the onramp integration suite that I disabled in favor of using E2E tests because I think that test is missing a couple items causing runs to be flakey. In particular, the expected event isn't being captured. |
| # This file contains metadata about published versions of this package | ||
| # This file is auto-generated for dependency resolution | ||
|
|
||
| [published.%s] |
There was a problem hiding this comment.
This file is a bit difficult to follow and it'll be harder to maintain over time. Let's please switch to a TOML library to handle the TOML parsing/serialization (e.g., go-toml: https://github.com/pelletier/go-toml) now that we have context on what it does 🙏
|
|
||
| [published.testnet] | ||
| chain-id = "4c78adac" | ||
| published-at = "0x5ef4b483da6644c84aa78eae4f51a9bfb1fb4554d5134ac98892e931fcbdd6bf" |
There was a problem hiding this comment.
Are these real testnet addresses? If not, I'd avoid committing them or the file
There was a problem hiding this comment.
this was a hack to make sure tests are passing here
we can try to remove this OR remove those test cases.
| treasury = "_" | ||
| mcms = "_" | ||
| mcms_owner = "_" | ||
| # [addresses] |
There was a problem hiding this comment.
We can clean all these commented part
|
|
||
| // UpdatePublishedTOMLForUpgrade updates an existing Published.toml file for a package upgrade. | ||
| // This updates the published-at address and increments the version. | ||
| func UpdatePublishedTOMLForUpgrade(packageDir, environment, chainID, newPublishedAt, originalID string, newVersion int, upgradeCap string) error { |
There was a problem hiding this comment.
Are UpdatePublishedTOMLForUpgrade, RemovePublishedTOML, WriteEphemeralPubFile and removePublishedEntry used? If not, we can remove them
| if !isZeroAddress(mcmsAddr) { | ||
| mcmsDir := filepath.Join(dstRoot, "mcms", "mcms") | ||
| if err := managePackage(mcmsDir, 1, rpcURL, env, mcmsAddr, mcmsAddr); err != nil { | ||
| if err := managePackage(mcmsDir, 1, rpcURL, env, mcmsAddr, mcmsAddr, pubfilePath); err != nil { |
There was a problem hiding this comment.
originalPkgId and latestPkgId seem to be always the same for every contract. I can't recall why we had it different. Could we just unify them?
There was a problem hiding this comment.
We can probably unify those. I think we have a test for an upgrade to an already upgraded package in please. IIRC that might have been the reason we had it there but not entirely sure.
FelixFan1992
changed the title
3 participants
No description provided.