Skip to content

feat(admin): ownership commands#320

Merged
xDarksome merged 4 commits into
mainfrom
feat/admin-cli-transfer-ownership
Feb 3, 2026
Merged

feat(admin): ownership commands#320
xDarksome merged 4 commits into
mainfrom
feat/admin-cli-transfer-ownership

Conversation

@xDarksome

@xDarksome xDarksome commented Feb 3, 2026

Copy link
Copy Markdown
Member

Description

Implements wcn_admin owner transfer and wcn_admin ownership accept commands.

Required to migrate the admin accounts to KMS/Safe.

How Has This Been Tested?

Manually

Due Diligence

  • Breaking change
  • Requires a documentation update
  • Requires a e2e/integration test update

@xDarksome xDarksome self-assigned this Feb 3, 2026
@claude

claude Bot commented Feb 3, 2026

Copy link
Copy Markdown

Claude finished @xDarksome's task in 58s —— View job


Code Review

  • Read changed files
  • Analyze code quality and security
  • Check for bugs and edge cases
  • Automated checks (external URLs, cache-control)
  • Post review findings

Issue 1: Typo in completion message

File: crates/admin_cli/src/ownership.rs:47

The completion message contains a grammatical error: "The complete the transfer" should be "To complete the transfer".

println!(
    "The complete the transfer the new owner is required to call `wcn_admin ownership accept` \
     command"
);

Issue 2: No ownership validation in accept_ownership

File: crates/admin_cli/src/ownership.rs:54-68

The accept_ownership function calls cluster.accept_ownership() without validating that the signer is the pending owner. While the smart contract should validate this, the CLI could provide better UX by checking the pending owner state before attempting the transaction and failing with a clearer error message.

Current implementation:

async fn accept_ownership(args: ClusterArgs) -> antml:parameter>
    let cluster = args.connect().await?;

    cluster
        .accept_ownership()
        .await
        .context("Cluster::accept_ownership")?;

    println!(
        "OK. {} is the new owner",
        cluster.smart_contract().signer().unwrap()
    );

    Ok(())
}

The .unwrap() on line 64 could panic if no signer is configured, though this is unlikely given the flow.


Issue 3: Missing ownership transfer validation in testing mock

File: crates/cluster/src/smart_contract/testing.rs:397-403

The transfer_ownership and accept_ownership implementations in the testing mock are stubs that don't actually update the ownership state. This means tests won't catch issues related to ownership transfer logic:

async fn transfer_ownership(&self, _new_owner: AccountAddress) -> WriteResult<()> {
    Ok(())
}

async fn accept_ownership(&self) -> WriteResult<()> {
    Ok(())
}

These should properly update this.sc_view.owner and validate the two-step ownership transfer process to match the real implementation.


Issue 4: No tests for ownership transfer functionality

No test files were added or modified for the new ownership transfer commands. Given this is security-critical functionality for migrating admin accounts to KMS/Safe, comprehensive tests should be added to verify:

  • Only owner can initiate transfer
  • Pending owner must accept
  • Unauthorized addresses cannot accept
  • Transfer completes correctly

@xDarksome xDarksome requested a review from mario-reown February 3, 2026 14:34
@xDarksome xDarksome merged commit 0e85df5 into main Feb 3, 2026
13 checks passed
@xDarksome xDarksome deleted the feat/admin-cli-transfer-ownership branch February 3, 2026 14:43
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