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.

Sign up for GitHub

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

ci: Debug CLI / test-solana job #573

Closed
wants to merge 11 commits into from
Closed

ci: Debug CLI / test-solana job #573

Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
3e1f0b5
solana: Revert `getVersion` on error to return `"2.0.0"`
nvsriram Dec 5, 2024
46b0b23
ci: Reset version and add logging to detect race condition
nvsriram Dec 6, 2024
3bdec10
ci: Reduce debugging to test race condition
nvsriram Dec 6, 2024
401cae1
ci: Remove pullChainConfig logs
nvsriram Dec 6, 2024
fb2acf7
ci: Check pullChainConfig and nttFromManager output
nvsriram Dec 6, 2024
364b49a
ci: Remove cyclic struct log
nvsriram Dec 6, 2024
ae13407
ci: Add deploySolana logs
nvsriram Dec 6, 2024
733c829
ci: Add nttFromManager logs
nvsriram Dec 6, 2024
24e0c2f
ci: Add sleep before fetching upgraded ntt
nvsriram Dec 6, 2024
9ca5212
ci: Add comments for sleep
nvsriram Dec 6, 2024
d44e84b
nit: Remove extra new line
nvsriram Dec 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions cli/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,6 +472,9 @@ yargs(hideBin(process.argv))
argv["binary"]
);

// we sleep here to ensure the program has enough time to be deployed
// and we can fetch the version from the program itself
await new Promise((resolve) => setTimeout(resolve, 2000));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a less arbitrary way to detect this? Can it be known that the program will always take less than 2 seconds to be deployed?

Copy link
Collaborator Author

@nvsriram nvsriram Dec 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There is no guarantee that the program will always take less than 2 seconds to be deployed or in this case for the RPC to catch up to the deployed program (I could probably update the comment).
This sleep happens after deploy proc has exited. So this sleep just acts as a buffer for the RPC to query the correct deployed program - similar to the sleep before initialize.

The less arbitrary option I considered was a spinloop inside nttFromManager if an optional expectedVersion parameter is passed in:

async function nttFromManager<N extends Network, C extends Chain>(
    ch: ChainContext<N, C>,
    nativeManagerAddress: string,
    expectedVersion?: string
): Promise<{ ntt: Ntt<N, C>; addresses: Partial<Ntt.Contracts> }> {
    let onlyManager: Ntt<N, C>;
    onlyManager = await ch.getProtocol("Ntt", {
        ...
    });
    while (expectedVersion && getVersion(ch.chain, onlyManager) !== expectedVersion) {
       await new Promise((resolve) => setTimeout(resolve, 2000));
       onlyManager = await ch.getProtocol("Ntt", {
         ...
       });
    }
    ...
}

And it could be called in upgrade:

const { ntt: upgraded } = await nttFromManager(ch, chainConfig.manager, toVersion ?? undefined);

Concerns with this approach:

  1. This CI job fail is only due to the edge case where the CLI release has not caught up to the latest NTT manager version so the complexity added with a spinloop and debugging might be overkill?
  2. Calling getProtocol in a loop might not be the most optimal way to fetch versions
  3. Not sure if the strict equality is the best way to compare versions (in case of semver etc.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think my proposal in #574 fixes the underlying problem without the arbitrary wait.

// reinit the ntt object to get the new version
// TODO: is there an easier way to do this?
const { ntt: upgraded } = await nttFromManager(ch, chainConfig.manager);
Expand Down Expand Up @@ -1342,6 +1345,7 @@ async function deploySolana<N extends Network, C extends SolanaChains>(

let binary: string;

// TODO: maybe this could be a flag?
const skipDeploy = false;

if (!skipDeploy) {
Expand Down
Loading