Skip to content

Merge main into release/v3.x-new to prepare for v3.6.11 release#417

Merged
dusan-maksimovic merged 7 commits intorelease/v3.x-newfrom
main
Apr 16, 2026
Merged

Merge main into release/v3.x-new to prepare for v3.6.11 release#417
dusan-maksimovic merged 7 commits intorelease/v3.x-newfrom
main

Conversation

@dusan-maksimovic
Copy link
Copy Markdown
Collaborator

Description

Closes: #XXXX

Add a description of the changes that this PR introduces and the files that
are the most critical to review.


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • Targeted the correct branch
  • Included the necessary unit tests
  • Added/adjusted the necessary interchain tests
  • Added necessary migration code for all stores that were adjusted or added
  • Added a changelog entry in .changelog
  • Compiled the contracts by using make compile and included content of the artifacts directory into the PR
  • Regenerated front-end schema by using make schema and included generated files into the PR
  • Updated the relevant documentation or specification
  • Reviewed "Files changed" and left comments if necessary
  • Confirmed all CI checks have passed

@dusan-maksimovic dusan-maksimovic self-assigned this Apr 16, 2026
@dusan-maksimovic dusan-maksimovic requested a review from a team as a code owner April 16, 2026 10:37
@dusan-maksimovic dusan-maksimovic merged commit d8e6ae7 into release/v3.x-new Apr 16, 2026
10 checks passed
@claude
Copy link
Copy Markdown

claude Bot commented Apr 16, 2026

Code Review — v3.6.11 Release Merge (main → release/v3.x-new)

This PR bundles two changes from main: a bug fix for MoveAdapterFunds (#395) and the standardisation of admin management across inflow adapters (#413). Notes below are retrospective but may be useful for follow-ups.


Bug Fix: MoveAdapterFunds with deposit denom (#395)

What changed: deposit_to_adapter gained a skip_vault_balance_check: bool parameter. When move_adapter_funds calls it, the flag is true because the vault's balance hasn't yet been replenished by the preceding withdraw message.

Overall: correct. CosmWasm executes messages atomically in order, so the withdraw will always run — and revert everything on failure — before the deposit is attempted. The regression test test_move_adapter_funds_succeeds_with_zero_vault_balance is well-written and documents the exact scenario that was broken.

Observation: The bool parameter is a code smell that makes the call-site meaning opaque (deposit_to_adapter(..., true)). A small refactor — extracting the balance-check guard as a separate step inside move_adapter_funds itself, or naming the parameter via a small enum — would make the intent clearer without changing behaviour. Not a blocker, but worth addressing in a subsequent cleanup.


Feature: Standardised admin interface (#413)

AddAdmin/RemoveAdmin/Admins have been promoted from CCTP-specific custom messages to the shared AdapterInterfaceMsg/AdapterInterfaceQueryMsg, and AddWhitelistAdmin/RemoveWhitelistAdmin have been added to the core Hydro contract.

✅ Things done well

  • Last-admin guard is present in every adapter and in the Hydro contract.
  • Duplicate-add and not-found checks are consistent.
  • Tests cover the happy path, unauthorised callers, duplicate adds, not-found removes, self-removal, and the last-admin guard — good coverage.
  • AdminsResponse is defined once in packages/interface and re-exported; no duplication.

Issues worth a follow-up

1. Authorization check placement is inconsistent in the skip adapter

In the skip adapter, validate_config_admin is called inside dispatch_execute_standard before delegating to execute_add_admin / execute_remove_admin:

// skip-adapter/src/contract.rs
AdapterInterfaceMsg::AddAdmin { admin_address } => {
    validate_config_admin(&deps, &info)?;  // auth here ...
    execute_add_admin(deps, info, admin_address)    // ... not inside
}

In the IBC and Mars adapters, the authorization check lives inside the function:

fn execute_add_admin(...) {
    validate_admin_caller(&deps.as_ref(), &info)?;  // auth inside
    ...
}

This inconsistency is not a bug (both paths protect the operation), but it means the skip adapter's execute_add_admin is internally unguarded. If the function were ever called from a second dispatch path, the guard would be silently bypassed. The IBC/Mars pattern (auth inside the function) is safer and should be adopted in the skip adapter too.

2. Hydro contract uses untyped string errors instead of structured variants

The adapter contracts correctly use structured error variants:

// ibc-adapter/src/error.rs
#[error("Admin already exists: {admin}")]
AdminAlreadyExists { admin: String },

The Hydro contract does not follow this pattern:

// contracts/hydro/src/contract.rs
return Err(ContractError::Std(StdError::generic_err(
    "Address is already a whitelist admin",
)));

Using StdError::generic_err for domain errors makes programmatic error handling harder for clients, and it's inconsistent with the rest of the codebase. Dedicated variants (WhitelistAdminAlreadyExists, WhitelistAdminNotFound) would be cleaner.

3. Missing test for removing the last whitelist admin (Hydro)

The adapter tests include test_remove_last_admin_fails. The Hydro test suite does not have an equivalent (test_remove_last_whitelist_admin_fails). The guard is present in the implementation, but the absence of the test leaves it uncovered.

4. Empty migration testing.rs files

All four new migration modules (cctp-adapter, ibc-adapter, mars-adapter, skip-adapter) ship an empty testing.rs. That's fine for a "no state migration" release, but the pattern of declaring #[cfg(test)] mod testing; pointing at an empty file is confusing — consider either omitting the test module entirely or adding a smoke test that calls migrate and asserts the version is updated (as done in some other contracts).

5. PR checklist is fully unchecked

For a release merge PR the checklist items don't all apply, but leaving every box unchecked makes the PR harder to audit. It would help to check the applicable items (compiled artifacts included, schema regenerated, changelog added) and explicitly mark the rest as N/A.


Minor

  • The #[allow(unused_imports)] on entry_point in every migration file is a known cosmwasm-schema limitation; the inline comment explains it, which is fine.
  • admins stored as Vec<Addr> and iterated with O(n) contains/retain — acceptable for the expected list sizes (admins are few), but a HashSet or BTreeSet-backed store would be more semantically correct if the list ever grows.

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