Feat/backend resolver api#454
Open
AbuTuraab wants to merge 18 commits into
Open
Conversation
…tract-shared-onchain-helpers refactor: extract shared authorization and storage helpers across onc…
…refactor/extract-shared-onchain-helpers Revert "refactor: extract shared authorization and storage helpers across onc…"
…tract-shared-onchain-helpers-v2 refactor: extract shared authorization and storage helpers across onc…
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a backend username resolver feature (DTO, service, module, controller, tests, and Jest config) and introduces shared on-chain utilities (auth/storage), TTL bumping for persistent storage, multiple contract refactors (core/factory/escrow/auction), expanded shared error enums, test reorganizations, and CI/workspace adjustments. ChangesBackend Resolver API
On-chain Shared Infrastructure & Contract Refactoring
sequenceDiagram
participant Client
participant Controller as ResolverController
participant Service as ResolverService
participant Repo as UsernameRepository
participant Contract as CoreContract
participant DB as Database
Client->>Controller: GET /resolve/:usernameHash
Controller->>Service: resolve(usernameHash)
Service->>Repo: findOne(usernameHash)
alt Cache Hit
Repo-->>Service: Username entity (cached)
Service-->>Controller: ResolvedUsername (from cache)
Controller-->>Client: 200 + resolved data
else Cache Miss
Repo-->>Service: undefined
Service->>Contract: resolve_stellar(usernameHash)
Contract-->>Service: stellarAddress
Service->>Contract: get_owner/registered_at/get_chain_address...
Contract-->>Service: owner, registeredAt, chain addresses
Service->>DB: save(Username entity)
DB-->>Service: persisted
Service-->>Controller: ResolvedUsername (fresh)
Controller-->>Client: 200 + resolved data
else Not Found
Repo-->>Service: undefined
Service->>Contract: resolve_stellar(usernameHash)
Contract-->>Service: error (not found)
Service-->>Controller: NotFoundException(usernameHash)
Controller-->>Client: 404 + message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
backend/package.json (1)
5-8: Consider adding start/run scripts.The
buildscript runstsc --noEmit(type-checking only, no emit). Consider adding:
- A
startorstart:devscript to run the NestJS application- A
compilescript to emit JavaScript todist/(e.g.,tsc)This may be intentional if build/start is managed at a monorepo root level.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/package.json` around lines 5 - 8, package.json currently only defines "build" (tsc --noEmit) and "test"; add runnable scripts so the NestJS app can be started and compiled: add a "compile" script that runs tsc (emit to dist) and a "start" or "start:dev" script that runs the NestJS entry (e.g., node dist/main.js or nest start --watch) depending on dev vs prod, and ensure "build" remains the type-checking alias if desired; update the "scripts" section to include "compile" and "start"/"start:dev" alongside the existing "build" and "test".backend/src/username/username.entity.ts (1)
4-27: Consider adding a database index for lookup performance.The
resolver.service.tsperformsfindOnelookups byusernameHash. Since this is the primary key (already indexed by default), lookups are efficient. However, if you expect queries by other fields (e.g.,stellarAddress,owner), consider adding indexes for those columns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/username/username.entity.ts` around lines 4 - 27, The entity Username currently only relies on the primary key usernameHash for indexed lookups; add database indexes for any other frequently queried columns (e.g., stellarAddress, owner, evmAddress) by adding appropriate `@Index` decorators to the Username class (or create composite indexes if queries filter on multiple fields) so resolver.service.ts and any findOne/find queries by those fields are faster; update migrations/schema accordingly to create those indexes in the database.backend/src/resolver/resolver.service.spec.ts (1)
1-95: Consider adding tests for theresolveStellarendpoint.The current tests only cover
resolve(). Per PR objectives, there's aGET /resolve/:usernameHash/stellarendpoint with its own service method (resolveStellar). Consider adding tests for:
- Cache hit returning Stellar address only
- Contract resolution for Stellar-only path
- Not-found handling in the Stellar-specific resolution path
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/resolver/resolver.service.spec.ts` around lines 1 - 95, Add unit tests for ResolverService.resolveStellar mirroring the existing resolve() specs: create tests for (1) cache hit returning only the stellarAddress and not calling coreContract.resolve_stellar or repository.save (use makeRepository(cached) with a Username containing stellarAddress and makeService), (2) cache miss where coreContract.resolve_stellar resolves to a Stellar address and repository.save is called with the usernameHash, stellarAddress, owner/registeredAt if needed (use makeRepository(null) and a coreContract with resolve_stellar mocked), and (3) contract-level not-found where coreContract.resolve_stellar rejects (mockRejectedValue new Error('ContractError: NotFound')) and the test expects a NotFoundException with the message `Username hash ${usernameHash} was not found on-chain` and repository.save not called; assert calls against coreContract.resolve_stellar and repository.save as in existing tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/resolver/resolver.service.ts`:
- Around line 39-48: The resolve(usernameHash) method treats any cached row with
a stellarAddress as a full cache hit, causing partial records (saved by the
stellar-only endpoint) to short-circuit resolution; update the logic in resolve
and the similar block around lines 51-64 to detect partial cache rows (e.g.,
missing owner, registeredAt, or chainAddresses) by checking fields returned from
findCached(usernameHash) and, if incomplete, call
resolveFromContract(usernameHash) to hydrate missing fields before saving via
usernames.save(...) and returning toResponse(...); alternatively persist and
check a completeness flag on the cached record so only fully-hydrated rows are
considered cache hits.
In `@onchain/contracts/core_contract/src/address_manager.rs`:
- Around line 148-153: The owner check currently calls
shared_auth::require_matching_auth(&env, &caller, &owner, CoreError::NotFound)
which causes an unauthorized caller to surface as NotFound; change the error
passed to require_matching_auth to CoreError::Unauthorized so the owner-mismatch
returns Unauthorized (leave the unwrap_or_panic on Registration::get_owner
as-is).
In `@onchain/contracts/escrow_contract/src/lib.rs`:
- Around line 62-66: The current unwrap_or_panic call uses
EscrowError::CommitmentNotRegistered which misreports a missing username when
the contract was never initialized; replace that error with a dedicated
initialization error (e.g. EscrowError::ContractNotInitialized or
EscrowError::NotInitialized) and use it in the call to
shared_auth::unwrap_or_panic around read_registration_contract(&env) (and update
the EscrowError enum to include the new variant and any matching
conversions/handlers); this ensures initialize() not having been called is
reported correctly rather than being conflated with CommitmentNotRegistered.
In `@onchain/contracts/factory_contract/src/lib.rs`:
- Around line 193-197: The config address used by
require_auction_contract_auth() is writable via the public configure() function,
allowing an attacker to repoint the auction contract and bypass auth for
deploy_username()/transfer_username(); fix this by adding an ownership/admin
check to configure() (or make configure() a one-time initializer) so only the
contract owner can change the auction address, and ensure
require_auction_contract_auth() continues to read the stored address
(read_auction_contract) and enforce that only the genuine auction contract
(shared_auth::require_address_auth) can trigger privileged actions.
In `@onchain/shared/src/lib.rs`:
- Around line 91-108: The helpers get_instance and set_instance currently
read/write critical instance state but never extend instance TTL; add an
instance-TTL extension call (env.storage().instance().extend_ttl(...)) after
reads/writes so the contract instance does not expire—either call the existing
extend_persistent_ttl policy or factor the same TTL logic into a new helper
(e.g., extend_instance_ttl) and invoke it from both get_instance and
set_instance to mirror the persistent storage TTL strategy.
---
Nitpick comments:
In `@backend/package.json`:
- Around line 5-8: package.json currently only defines "build" (tsc --noEmit)
and "test"; add runnable scripts so the NestJS app can be started and compiled:
add a "compile" script that runs tsc (emit to dist) and a "start" or "start:dev"
script that runs the NestJS entry (e.g., node dist/main.js or nest start
--watch) depending on dev vs prod, and ensure "build" remains the type-checking
alias if desired; update the "scripts" section to include "compile" and
"start"/"start:dev" alongside the existing "build" and "test".
In `@backend/src/resolver/resolver.service.spec.ts`:
- Around line 1-95: Add unit tests for ResolverService.resolveStellar mirroring
the existing resolve() specs: create tests for (1) cache hit returning only the
stellarAddress and not calling coreContract.resolve_stellar or repository.save
(use makeRepository(cached) with a Username containing stellarAddress and
makeService), (2) cache miss where coreContract.resolve_stellar resolves to a
Stellar address and repository.save is called with the usernameHash,
stellarAddress, owner/registeredAt if needed (use makeRepository(null) and a
coreContract with resolve_stellar mocked), and (3) contract-level not-found
where coreContract.resolve_stellar rejects (mockRejectedValue new
Error('ContractError: NotFound')) and the test expects a NotFoundException with
the message `Username hash ${usernameHash} was not found on-chain` and
repository.save not called; assert calls against coreContract.resolve_stellar
and repository.save as in existing tests.
In `@backend/src/username/username.entity.ts`:
- Around line 4-27: The entity Username currently only relies on the primary key
usernameHash for indexed lookups; add database indexes for any other frequently
queried columns (e.g., stellarAddress, owner, evmAddress) by adding appropriate
`@Index` decorators to the Username class (or create composite indexes if queries
filter on multiple fields) so resolver.service.ts and any findOne/find queries
by those fields are faster; update migrations/schema accordingly to create those
indexes in the database.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 82b7a566-b43b-4ab1-bc10-98191bcb501e
⛔ Files ignored due to path filters (1)
backend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
backend/.gitignorebackend/jest.config.jsbackend/package.jsonbackend/src/resolver/dto/resolve-username.dto.tsbackend/src/resolver/resolver.controller.tsbackend/src/resolver/resolver.module.tsbackend/src/resolver/resolver.service.spec.tsbackend/src/resolver/resolver.service.tsbackend/src/username/username.entity.tsbackend/tsconfig.jsononchain/contracts/auction_contract/src/lib.rsonchain/contracts/auction_contract/src/storage.rsonchain/contracts/core_contract/src/address_manager.rsonchain/contracts/core_contract/src/admin.rsonchain/contracts/core_contract/src/storage.rsonchain/contracts/escrow_contract/src/lib.rsonchain/contracts/escrow_contract/src/storage.rsonchain/contracts/factory_contract/src/lib.rsonchain/contracts/factory_contract/src/storage.rsonchain/shared/src/lib.rs
Contributor
ryzen-xp
added
CI Failed
|
@AbuTuraab Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
onchain/contracts/core_contract/src/address_manager.rs (2)
160-175:⚠️ Potential issue | 🔴 Critical
remove_stellar_addressuses undeclared variables.
addresses_key(line 160) andprimary_key(lines 170, 175) are referenced but not defined in this function. These variables are declared in a preceding function and are out of scope. The code will not compile as written.Suggested fix
pub fn remove_stellar_address( env: Env, caller: Address, username_hash: BytesN<32>, stellar_address: Address, ) { + let addresses_key = storage::DataKey::StellarAddresses(username_hash.clone()); + let primary_key = storage::DataKey::StellarAddress(username_hash.clone()); + caller.require_auth();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/core_contract/src/address_manager.rs` around lines 160 - 175, In remove_stellar_address the variables addresses_key and primary_key are out of scope; recreate the proper storage keys inside this function using the same key construction as the other function: build addresses_key (e.g., storage::DataKey::StellarAddresses(username_hash.clone())) and primary_key (e.g., storage::DataKey::StellarAddress(username_hash.clone())) before using them, then call shared_storage::set_persistent/remove with those local keys; ensure you use the same username_hash variable and types as used when reading/updating `updated` and `primary`.
19-37:⚠️ Potential issue | 🔴 CriticalAdd missing
require_auth()calls to chain-address mutations.Both
add_chain_addressandremove_chain_addressaccept acallerparameter but never verify it withrequire_auth(). In Soroban SDK, passing an Address argument does not automatically authenticate it—the contract must explicitly enforce authorization. This allows an attacker to pass the owner's address ascallerand satisfy theowner == callercheck without proof of control, enabling unauthorized mutations.Suggested fix
pub fn add_chain_address( env: Env, caller: Address, username_hash: BytesN<32>, chain: ChainType, address: Bytes, ) { + caller.require_auth(); + let owner_key = CommitmentKey::Commitment(username_hash.clone());pub fn remove_chain_address( env: Env, caller: Address, username_hash: BytesN<32>, chain: ChainType, ) { + caller.require_auth(); + let owner_key = CommitmentKey::Commitment(username_hash.clone());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/core_contract/src/address_manager.rs` around lines 19 - 37, The add_chain_address and remove_chain_address functions accept a caller Address but never authenticate it; call the Soroban Env require_auth for the provided caller at the start of each function (before the owner equality and permission checks) to ensure the caller actually signed the transaction. Locate add_chain_address and remove_chain_address in address_manager.rs and invoke env.require_auth on the caller parameter (using the Env require_auth API) immediately after function entry so owner == caller cannot be spoofed and permission checks remain valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@onchain/contracts/auction_contract/src/lib.rs`:
- Around line 92-100: The call to require_status currently passes an extra &env
and discards its Result; update the require_status invocation for the Closed
check to call require_status(status, types::AuctionStatus::Closed,
errors::AuctionError::NotClosed)? (remove the &env), propagate errors with the
trailing ? and then delete the subsequent manual if status !=
types::AuctionStatus::Closed check and its Err return so the function compiles
and uses the helper correctly.
In `@onchain/contracts/core_contract/src/admin.rs`:
- Around line 69-74: The code in update_smt_root is requiring both the owner and
the operator (shared_auth::require_address_auth(&owner) plus
operator.require_auth()), which forces a 2-party signature; to restore the
intended operator-only behavior remove the owner auth check so only the
operator's authority is required (keep storage::get_operator(&env) and
operator.require_auth()), or if the intent is to allow either party to act
implement a single-check branch that calls
shared_auth::require_address_auth(&owner) OR operator.require_auth() (instead of
requiring both). Adjust the logic around shared_auth::unwrap_or_panic/get_owner
and storage::get_operator accordingly so update_smt_root enforces the correct
single-party authorization.
- Line 26: The file uses shared_auth::unwrap_or_panic (called in the admin code
alongside storage::get_owner and CoreError::NotFound) but never imports the
shared auth module; add the missing import at the top of the file (for example:
use shared::auth as shared_auth;) so shared_auth::unwrap_or_panic resolves and
the module compiles.
In `@onchain/contracts/factory_contract/src/storage.rs`:
- Around line 92-94: get_config currently performs a non-bumping read which lets
DataKey::Config expire after inactivity; update get_config to refresh the TTL on
reads by using the storage API's bumping-read (or by calling the equivalent bump
method after reading) so the DeployConfig stays alive—replace the call to
shared_storage::get_persistent(env, &DataKey::Config) in get_config with the
bumping variant (e.g., shared_storage::get_persistent_bump(...) or call
shared_storage::bump_persistent(env, &DataKey::Config) immediately after
retrieving the value) and keep the return type as Option<DeployConfig>; this
keeps DataKey::Config TTL refreshed without changing set_config.
---
Outside diff comments:
In `@onchain/contracts/core_contract/src/address_manager.rs`:
- Around line 160-175: In remove_stellar_address the variables addresses_key and
primary_key are out of scope; recreate the proper storage keys inside this
function using the same key construction as the other function: build
addresses_key (e.g., storage::DataKey::StellarAddresses(username_hash.clone()))
and primary_key (e.g., storage::DataKey::StellarAddress(username_hash.clone()))
before using them, then call shared_storage::set_persistent/remove with those
local keys; ensure you use the same username_hash variable and types as used
when reading/updating `updated` and `primary`.
- Around line 19-37: The add_chain_address and remove_chain_address functions
accept a caller Address but never authenticate it; call the Soroban Env
require_auth for the provided caller at the start of each function (before the
owner equality and permission checks) to ensure the caller actually signed the
transaction. Locate add_chain_address and remove_chain_address in
address_manager.rs and invoke env.require_auth on the caller parameter (using
the Env require_auth API) immediately after function entry so owner == caller
cannot be spoofed and permission checks remain valid.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aaa25753-89d1-4e2a-a8cb-b203522a4da9
⛔ Files ignored due to path filters (1)
backend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
backend/.gitignorebackend/src/resolver/resolver.controller.tsbackend/tsconfig.jsononchain/contracts/auction_contract/src/lib.rsonchain/contracts/auction_contract/src/storage.rsonchain/contracts/core_contract/src/address_manager.rsonchain/contracts/core_contract/src/admin.rsonchain/contracts/core_contract/src/storage.rsonchain/contracts/escrow_contract/src/lib.rsonchain/contracts/escrow_contract/src/storage.rsonchain/contracts/factory_contract/src/lib.rsonchain/contracts/factory_contract/src/storage.rs
✅ Files skipped from review due to trivial changes (3)
- backend/.gitignore
- backend/src/resolver/resolver.controller.ts
- backend/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (5)
- onchain/contracts/factory_contract/src/lib.rs
- onchain/contracts/core_contract/src/storage.rs
- onchain/contracts/escrow_contract/src/lib.rs
- onchain/contracts/escrow_contract/src/storage.rs
- onchain/contracts/auction_contract/src/storage.rs
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
onchain/contracts/factory_contract/src/lib.rs (2)
27-125:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winResolve the merge conflicts before merging this contract.
initialize,configure,set_admin,set_operator, anddeploy_usernameare still half-merged here. In its current state the contract does not compile, and it's impossible to tell which auth/storage path is the intended one.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@onchain/contracts/factory_contract/src/lib.rs` around lines 27 - 125, The file contains unresolved merge conflicts leaving duplicated/partial implementations of initialize, configure, set_admin, set_operator, and deploy_username; resolve by choosing and consolidating one coherent implementation for each function (e.g., the refactored signatures that return Result<(), FactoryError> for initialize, set_admin, set_operator and the updated parameter list for initialize including admin, oprator, core_wasm_hash), remove leftover HEAD conflict markers and the old variants (including the configure and deploy_username halves), ensure auth checks use the chosen error-return style (ok_or/require_auth) and that storage setters (set_owner, set_admin, set_operator, set_core_wasm_hash, set_auction_contract, set_core_contract, set_username) and events (emit_username_deployed or env.events().publish) are present consistently so the final code compiles.
128-138:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
transfer_usernameis currently permissionless.This path updates
record.ownerand persists it without requiring auth from the current owner, auction contract, admin, or operator. Any caller can take over any stored username.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@onchain/contracts/factory_contract/src/lib.rs` around lines 128 - 138, The transfer_username function currently allows anyone to change ownership; fix it by checking the caller identity before mutating state: retrieve the caller (env.invoker or equivalent) and ensure it equals the current owner (record.owner) or matches an allowed authority (the configured auction contract address, the stored admin address, or an operator mapping) and return Err(FactoryError::Unauthorized) if not authorized; only after this authorization check proceed to update record.owner, call set_username, and emit_ownership_transferred so transfer_username enforces proper permissions.onchain/contracts/core_contract/src/address_manager.rs (1)
234-243:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix enum/match mismatch in
validate_address—code will not compile.The
ChainTypeenum definesEthereum,Tron,Stellar,Bnb, andOther, but the match statement references non-existent variants (ChainType::EvmandChainType::Cosmos) and lacks arms forTron,Stellar,Bnb, andOther. The test on line 89 usesChainType::Ethereum, which doesn't match the expectedEvmvariant. Update the match to handle all enum variants, fix the variant name (Evm→Ethereum), and provide validation logic for missing chains.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@onchain/contracts/core_contract/src/address_manager.rs` around lines 234 - 243, The match in validate_address must be updated to use the actual ChainType variants and cover all arms: replace ChainType::Evm with ChainType::Ethereum and add match arms for ChainType::Tron, ChainType::Stellar, ChainType::Bnb and ChainType::Other (or a catch-all _) so the function compiles; keep the existing Ethereum logic (42 bytes and leading '0x' check using address.get(0) == Some(0x30) && address.get(1) == Some(0x78)), implement reasonable validation for the new arms (e.g., Tron: typical base58/hex length range similar to Bitcoin, Stellar: expected length range for StrKey/public key, Bnb: expected Bech32/length range) or conservative byte-length checks, and return false for Other if unknown; update validate_address to include these arms and return a bool for every ChainType variant.
🧹 Nitpick comments (3)
onchain/contracts/auction_contract/src/test.rs (1)
1-1: 💤 Low valueRemove the leading UTF-8 BOM from this file.
Line 1 begins with a
U+FEFF(BOM) byte before#[cfg(test)]. While modernrustctolerates a leading BOM, it confuses many tools (diff viewers, formatters, some editors) and is inconsistent with the rest of the codebase. Strip it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@onchain/contracts/auction_contract/src/test.rs` at line 1, The file begins with a UTF-8 BOM (U+FEFF) immediately before the #[cfg(test)] token which can confuse tools; open onchain/contracts/auction_contract/src/test.rs and remove the leading BOM character so the file starts directly with #[cfg(test)] (save the file as UTF-8 without BOM or re-encode/save via your editor/IDE or run a cleanup that strips U+FEFF from the file start).onchain/contracts/factory_contract/src/types.rs (1)
13-13: 💤 Low valueMissing trailing comma on
CoreWasm.The other variants in this enum carry trailing commas;
CoreWasmdoes not.rustfmtwill normalize this, but it's worth fixing in the same patch.📝 Suggested fix
- CoreWasm + CoreWasm,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@onchain/contracts/factory_contract/src/types.rs` at line 13, The enum variant CoreWasm is missing a trailing comma; update the enum in types.rs so the variant CoreWasm ends with a comma (e.g., change "CoreWasm" to "CoreWasm,") to match the other variants and satisfy rustfmt and style consistency.onchain/contracts/core_contract/src/events.rs (1)
11-63: ⚖️ Poor tradeoffPervasive
#[allow(deprecated)]masks the migration; prefer the#[contractevent]pattern used elsewhere in this PR.Every helper here is annotated
#[allow(deprecated)]becauseenv.events().publish((sym,), tuple)is the legacy API; the rest of the PR (e.g.,escrow_contract/src/events.rs’sVaultCrtEvent) has migrated to the typed#[contractevent]form, which gives you compile-checked topic/data layouts and avoids the suppression. Unless there is a specific reason the core contract must stay on the legacy API, please convert these to typed events for consistency and to drop the warnings altogether.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@onchain/contracts/core_contract/src/events.rs` around lines 11 - 63, The helpers currently use the deprecated env.events().publish((sym,), ...) pattern and suppress warnings with #[allow(deprecated)]; convert each legacy helper (emit_primary_set, emit_wallet_added, emit_wallet_removed, emit_escrow_created, emit_escrow_released, emit_escrow_refunded, emit_ownership_transferred, emit_send) to the typed #[contractevent] pattern used elsewhere (e.g., VaultCrtEvent) by defining a #[contractevent] struct per event that declares its topic/data layout (matching the EVT_* intent), remove the #[allow(deprecated)] annotations, and emit them via the typed events API (construct the event struct and publish it through env.events() using the contractevent-compatible publish method) so the compiler can validate the layout and warnings are gone.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@onchain/contracts/auction_contract/src/test.rs`:
- Around line 7-23: The module defines duplicate items causing compilation error
E0428: remove the redundant definitions so only one DummyFactory struct (marked
with #[contract]) and one setup function remain; specifically, keep the
centralized DummyFactory and setup near the top (the ones currently used by
tests) and delete the later duplicate definitions of DummyFactory and setup
found further down the test module so there are no repeated symbols.
In `@onchain/contracts/core_contract/src/events.rs`:
- Around line 1-63: The file is missing three event symbols and helper functions
that admin.rs imports: add constants EVT_INIT, EVT_ROLE_GRANT, EVT_ROOT_UPD
using symbol_short! and implement emit_init(env: &Env, owner: &Address),
emit_role_granted(env: &Env, role: &Symbol, grantee: &Address, granted_by:
&Address), and emit_root_updated(env: &Env, old_root: &Address, new_root:
&Address) following the same env.events().publish(...) pattern as the existing
emit_* helpers (use clones where needed) so the names emit_init,
emit_role_granted, and emit_root_updated resolve; alternatively, if you intend
not to emit these typed events remove the imports and calls from admin.rs.
In `@onchain/contracts/core_contract/src/lib.rs`:
- Around line 274-310: The create_escrow function currently accepts any
release_at; add a validation in create_escrow that compares release_at to the
current time (env.ledger().timestamp()) and returns a new CoreError variant
(e.g., InvalidReleaseTime or ReleaseTimeNotInFuture) if release_at <= now; add
the corresponding error variant to errors.rs, and ensure the function returns
Err(CoreError::InvalidReleaseTime) before transferring tokens, incrementing the
counter, or writing the EscrowRecord so no side-effects occur when release_at is
not in the future.
- Around line 44-113: Unresolved merge conflict markers remain and leave two
conflicting impl blocks (impl Contract { ... } referencing
Admin/Registration/Resolver/AddressManager/Transfer/Proof/PublicSignals/PrivacyMode/PermissionSet/Bytes
and impl CoreContract { __constructor(...) }) which prevents compilation; remove
the entire legacy HEAD block (the impl Contract { ... } delegating to
Admin/Registration/Resolver/AddressManager/Transfer) and the conflict markers
(<<<<<<<, =======, >>>>>>>) so only the new impl CoreContract with
__constructor(env, owner, username_hash) remains, and then update/remove any
leftover references or imports for symbols like Admin, Registration, Resolver,
AddressManager, Transfer, Proof, PublicSignals, PrivacyMode, PermissionSet,
Bytes or errors::CoreError to match the refactor (e.g., use
crate::errors::CoreError if needed).
- Around line 259-272: The code currently calls peer_core.resolve() which will
trap on peer contract errors; replace that call with peer_core.try_resolve()?
and propagate/mapping the Err into your domain error (e.g., map_err(|e|
CoreError::PeerResolveFailed(e.to_string())) or another existing CoreError
variant) so send_to_username returns an Err instead of aborting; update the call
site in send_to_username to use try_resolve(), handle Result<Address,
soroban_sdk::Error> and then proceed with token.transfer and emit_send on
success.
In `@onchain/contracts/core_contract/src/registration.rs`:
- Around line 70-73: The get_owner function currently does a plain persistent
read (using e.storage().persistent().get(&key)) so entries won't have their TTL
refreshed; replace that plain read with the existing "bumping" persistent lookup
used elsewhere to refresh the registration TTL on read (i.e., call the bumping
read helper with the Env and DataKey::Commitment key instead of .get), leaving
the signature pub fn get_owner(e: Env, commitment: BytesN<32>) ->
Option<Address> and returning the same Option<Address>.
In `@onchain/contracts/core_contract/src/types.rs`:
- Around line 17-25: The on-chain ChainType enum no longer matches the backend
resolver keys, causing get_chain_address(usernameHash, ...) calls
(resolver.service.ts expecting 'EVM','Bitcoin','Solana','Cosmos') to fail; fix
by reconciling both sides: either restore enum variants Evm and Cosmos in the
ChainType enum (replace/alias Ethereum->Evm, add Cosmos) so the contract matches
the backend, or update the backend resolver.service.ts to map its emitted chain
keys ('EVM' -> 'Ethereum' or 'Evm', 'Cosmos' -> 'Other' or specific chains) and
update any API docs; focus your change on the ChainType enum declaration and the
resolver.service.ts mapping logic and ensure tests cover get_chain_address
lookups for 'EVM' and 'Cosmos'.
In `@onchain/contracts/factory_contract/src/lib.rs`:
- Around line 157-190: deploy_core currently trusts the caller-supplied resolver
(resolver.require_auth()) and thus allows any caller to pass a resolver they
control and overwrite username state; fix by ensuring the caller is the resolver
and preventing overwrites: add a check that resolver == e.invoker() (or
otherwise assert the invoker matches) and call resolver.require_auth() only
after that, and also check for existing registrations by calling the lookup used
elsewhere (e.g., get_core_contract or get_username) and return an error (e.g.,
FactoryError::AlreadyRegistered) if a record already exists before calling
set_core_contract, set_username, or emit_username_deployed (refer to
deploy_core, resolver.require_auth(), UsernameRecord, set_core_contract,
set_username, emit_username_deployed).
In `@onchain/contracts/factory_contract/src/storage.rs`:
- Around line 6-29: Remove the leftover Git conflict markers (<<<<<<<, =======,
>>>>>>>) and restore the intended unified content in storage.rs: keep the TTL
constants PERSISTENT_BUMP_AMOUNT and PERSISTENT_LIFETIME_THRESHOLD and the
storage accessors such as set_auction_contract, get_auction_contract (and any
other CoreContract-related setters/getters around lines 60-80) ensuring there
are no duplicate definitions; pick the correct implementation for
DataKey::AuctionContract and the CoreContract storage model, delete the conflict
markers, and run a quick compile to verify the module builds.
In `@onchain/contracts/factory_contract/src/types.rs`:
- Around line 5-14: There is a merge conflict that left storage.rs using the old
zero-arg API for set_core_contract/get_core_contract while other changes expect
the new signature that requires a username: BytesN<32>; resolve the conflict by
keeping the new API in storage.rs (change set_core_contract and
get_core_contract to accept username: BytesN<32> and update internal key
construction to use DataKey::CoreContract(username) or
DataKey::Username(username) as appropriate), then update all call sites (e.g.,
lib.rs where set_core_contract(&env, &core_contract) is used) to pass the
username hash (BytesN<32>) as the additional first/second argument to match the
new signature (e.g., set_core_contract(&env, username_hash.clone(),
&core_contract)); ensure types match (BytesN<32>) and adjust imports/variables
so compilation succeeds.
- Around line 27-30: DeployConfig is currently unused and marked
#[allow(dead_code)], but if activated the field rename from admin to resolver is
a breaking schema change; update or add a migration path in the storage
accessors (get_config, set_config) to detect and convert old stored blobs using
the old admin field to the new resolver field before deserializing, or implement
versioned config records with a schema_version field and a migration routine
applied on load; also update factory.md to replace references to admin with
resolver and document the migration/activation steps so existing persistent data
is handled safely.
---
Outside diff comments:
In `@onchain/contracts/core_contract/src/address_manager.rs`:
- Around line 234-243: The match in validate_address must be updated to use the
actual ChainType variants and cover all arms: replace ChainType::Evm with
ChainType::Ethereum and add match arms for ChainType::Tron, ChainType::Stellar,
ChainType::Bnb and ChainType::Other (or a catch-all _) so the function compiles;
keep the existing Ethereum logic (42 bytes and leading '0x' check using
address.get(0) == Some(0x30) && address.get(1) == Some(0x78)), implement
reasonable validation for the new arms (e.g., Tron: typical base58/hex length
range similar to Bitcoin, Stellar: expected length range for StrKey/public key,
Bnb: expected Bech32/length range) or conservative byte-length checks, and
return false for Other if unknown; update validate_address to include these arms
and return a bool for every ChainType variant.
In `@onchain/contracts/factory_contract/src/lib.rs`:
- Around line 27-125: The file contains unresolved merge conflicts leaving
duplicated/partial implementations of initialize, configure, set_admin,
set_operator, and deploy_username; resolve by choosing and consolidating one
coherent implementation for each function (e.g., the refactored signatures that
return Result<(), FactoryError> for initialize, set_admin, set_operator and the
updated parameter list for initialize including admin, oprator, core_wasm_hash),
remove leftover HEAD conflict markers and the old variants (including the
configure and deploy_username halves), ensure auth checks use the chosen
error-return style (ok_or/require_auth) and that storage setters (set_owner,
set_admin, set_operator, set_core_wasm_hash, set_auction_contract,
set_core_contract, set_username) and events (emit_username_deployed or
env.events().publish) are present consistently so the final code compiles.
- Around line 128-138: The transfer_username function currently allows anyone to
change ownership; fix it by checking the caller identity before mutating state:
retrieve the caller (env.invoker or equivalent) and ensure it equals the current
owner (record.owner) or matches an allowed authority (the configured auction
contract address, the stored admin address, or an operator mapping) and return
Err(FactoryError::Unauthorized) if not authorized; only after this authorization
check proceed to update record.owner, call set_username, and
emit_ownership_transferred so transfer_username enforces proper permissions.
---
Nitpick comments:
In `@onchain/contracts/auction_contract/src/test.rs`:
- Line 1: The file begins with a UTF-8 BOM (U+FEFF) immediately before the
#[cfg(test)] token which can confuse tools; open
onchain/contracts/auction_contract/src/test.rs and remove the leading BOM
character so the file starts directly with #[cfg(test)] (save the file as UTF-8
without BOM or re-encode/save via your editor/IDE or run a cleanup that strips
U+FEFF from the file start).
In `@onchain/contracts/core_contract/src/events.rs`:
- Around line 11-63: The helpers currently use the deprecated
env.events().publish((sym,), ...) pattern and suppress warnings with
#[allow(deprecated)]; convert each legacy helper (emit_primary_set,
emit_wallet_added, emit_wallet_removed, emit_escrow_created,
emit_escrow_released, emit_escrow_refunded, emit_ownership_transferred,
emit_send) to the typed #[contractevent] pattern used elsewhere (e.g.,
VaultCrtEvent) by defining a #[contractevent] struct per event that declares its
topic/data layout (matching the EVT_* intent), remove the #[allow(deprecated)]
annotations, and emit them via the typed events API (construct the event struct
and publish it through env.events() using the contractevent-compatible publish
method) so the compiler can validate the layout and warnings are gone.
In `@onchain/contracts/factory_contract/src/types.rs`:
- Line 13: The enum variant CoreWasm is missing a trailing comma; update the
enum in types.rs so the variant CoreWasm ends with a comma (e.g., change
"CoreWasm" to "CoreWasm,") to match the other variants and satisfy rustfmt and
style consistency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 78155a4a-4a82-4e15-afc5-52ec4ff6fd1f
⛔ Files ignored due to path filters (1)
onchain/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (38)
.codex.gitignoreonchain/Cargo.tomlonchain/contracts/auction_contract/src/lib.rsonchain/contracts/auction_contract/src/test.rsonchain/contracts/auction_contract/src/types.rsonchain/contracts/core_contract/SECURITY_NOTE.mdonchain/contracts/core_contract/core.mdonchain/contracts/core_contract/src/address_manager.rsonchain/contracts/core_contract/src/admin.rsonchain/contracts/core_contract/src/alien_gateway.rsonchain/contracts/core_contract/src/alien_gateway/storage.rsonchain/contracts/core_contract/src/errors.rsonchain/contracts/core_contract/src/events.rsonchain/contracts/core_contract/src/lib.rsonchain/contracts/core_contract/src/registration.rsonchain/contracts/core_contract/src/resolver.rsonchain/contracts/core_contract/src/smt_root.rsonchain/contracts/core_contract/src/storage.rsonchain/contracts/core_contract/src/test.rsonchain/contracts/core_contract/src/test_address_manager.rsonchain/contracts/core_contract/src/test_registration.rsonchain/contracts/core_contract/src/transfer.rsonchain/contracts/core_contract/src/types.rsonchain/contracts/core_contract/src/zk_verifier.rsonchain/contracts/escrow_contract/src/events.rsonchain/contracts/escrow_contract/src/lib.rsonchain/contracts/escrow_contract/src/storage.rsonchain/contracts/escrow_contract/src/test.rsonchain/contracts/factory_contract/src/lib.rsonchain/contracts/factory_contract/src/storage.rsonchain/contracts/factory_contract/src/test.rsonchain/contracts/factory_contract/src/types.rsonchain/shared/src/errors.rsonchain/shared/src/lib.rsonchain/tests/Cargo.tomlonchain/tests/e2e/mock_registration_contract.rsonchain/tests/e2e/test_full_flow.rs
💤 Files with no reviewable changes (12)
- onchain/contracts/core_contract/src/smt_root.rs
- onchain/contracts/core_contract/core.md
- onchain/contracts/core_contract/src/zk_verifier.rs
- onchain/contracts/core_contract/src/transfer.rs
- onchain/tests/e2e/test_full_flow.rs
- onchain/contracts/core_contract/src/test_address_manager.rs
- onchain/tests/e2e/mock_registration_contract.rs
- onchain/contracts/core_contract/src/test_registration.rs
- onchain/tests/Cargo.toml
- onchain/contracts/core_contract/src/alien_gateway.rs
- onchain/contracts/core_contract/SECURITY_NOTE.md
- onchain/contracts/core_contract/src/alien_gateway/storage.rs
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- onchain/contracts/core_contract/src/errors.rs
- onchain/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (4)
- onchain/contracts/escrow_contract/src/storage.rs
- onchain/contracts/escrow_contract/src/lib.rs
- onchain/contracts/core_contract/src/storage.rs
- onchain/shared/src/lib.rs
There was a problem hiding this comment.
♻️ Duplicate comments (3)
onchain/contracts/escrow_contract/src/lib.rs (1)
46-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a dedicated “not initialized” error instead of domain/authorization errors.
At Line 103-104, missing
registration_contractcurrently returnsEscrowError::CommitmentNotRegistered, and at Line 46 / Line 54, missing admin maps toEscrowError::Unauthorized. These paths represent contract setup state, not caller/domain failures, and mislead clients/operators during recovery.Suggested fix
diff --git a/onchain/contracts/escrow_contract/src/lib.rs b/onchain/contracts/escrow_contract/src/lib.rs @@ - let old_admin = read_escrow_admin(&env).ok_or(EscrowError::Unauthorized)?; + let old_admin = read_escrow_admin(&env).ok_or(EscrowError::NotInitialized)?; @@ - let admin = read_escrow_admin(&env).ok_or(EscrowError::Unauthorized)?; + let admin = read_escrow_admin(&env).ok_or(EscrowError::NotInitialized)?; @@ - let registration = - read_registration_contract(&env).ok_or(EscrowError::CommitmentNotRegistered)?; + let registration = + read_registration_contract(&env).ok_or(EscrowError::NotInitialized)?;Also add
NotInitialized(orContractNotInitialized) toonchain/contracts/escrow_contract/src/errors.rswith a distinct code.Also applies to: 54-55, 103-104
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@onchain/contracts/escrow_contract/src/lib.rs` around lines 46 - 47, The code treats missing contract setup state as domain errors; add a new EscrowError variant (e.g., NotInitialized or ContractNotInitialized) in onchain/contracts/escrow_contract/src/errors.rs with a unique error code, then replace uses of .ok_or(EscrowError::Unauthorized) on read_escrow_admin (function read_escrow_admin) and .ok_or(EscrowError::CommitmentNotRegistered) on read_registration_contract (function read_registration_contract) to return the new NotInitialized error instead, and update any error handling/docs/tests accordingly so contract-not-initialized paths are clearly distinguished from authorization/commitment failures.onchain/contracts/core_contract/src/lib.rs (2)
208-244:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
release_atis in the future before creating the escrow.
create_escrowaccepts anyrelease_atand stores it verbatim. If a caller passes a value<= env.ledger().timestamp(), the resulting escrow is immediately releasable in the same/next ledger viarelease_escrow, undermining the time-locked semantics. Reject non-future timestamps before transferring tokens or persisting state.🔧 Proposed fix
if amount <= 0 { return Err(CoreError::InvalidAmount); } + if release_at <= env.ledger().timestamp() { + return Err(CoreError::InvalidReleaseTime); + } let owner = get_owner(&env).ok_or(CoreError::Unauthorized)?; owner.require_auth();(Add the
InvalidReleaseTimevariant inerrors.rsif it does not already exist.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@onchain/contracts/core_contract/src/lib.rs` around lines 208 - 244, The create_escrow function allows release_at ≤ current timestamp which breaks time-locking; add a check at the top of create_escrow that compares release_at against env.ledger().timestamp() and returns Err(CoreError::InvalidReleaseTime) when release_at is not strictly in the future (<= now) before performing owner auth, token.transfer, or set_escrow; ensure an InvalidReleaseTime variant exists in CoreError (add it in errors.rs if missing) so callers receive a clear error.
193-205:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUse
try_resolve()to propagate peer errors instead of trapping.
peer_core.resolve()at line 199 invokes the infallible client variant; sincePeerCoreInterface::resolvereturnsResult<Address, CoreError>, any peer-side error (e.g.,NoAddressLinked) traps the calling contract and aborts the entiresend_to_usernametransaction with no chance to surface a domain-specific error. Switch totry_resolve()and map the error to aCoreErrorvariant.🔧 Suggested fix
- let peer_core = PeerCoreClient::new(&env, &peer_core_addr); - let recipient = peer_core.resolve(); + let peer_core = PeerCoreClient::new(&env, &peer_core_addr); + let recipient = peer_core + .try_resolve() + .map_err(|_| CoreError::NoAddressLinked)? + .map_err(|e| e)?;(Adjust the error mapping to suit your
CoreErrorvariants; the outerResultis theResult<_, soroban_sdk::Error>from invocation, the inner is the contract'sResult<Address, CoreError>.)soroban-sdk 23 try_ generated client method nested Result return type for contracts returning Result<T, ContractError>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@onchain/contracts/core_contract/src/lib.rs` around lines 193 - 205, The code currently calls PeerCoreClient::resolve() which uses the infallible client and will trap on peer-side errors; change to PeerCoreClient::try_resolve() and propagate/map the inner contract error into your CoreError (e.g., map_err(|e| CoreError::NoAddressLinked) or appropriate variant) so send_to_username returns a domain error instead of aborting; update the variable assignment (recipient) to handle the nested Result and use the mapped Address on success before calling TokenClient::transfer and emit_send.
🧹 Nitpick comments (1)
onchain/contracts/core_contract/src/lib.rs (1)
84-118: 💤 Low valueAvoid loading wallet labels twice in
add_wallet.When
is_newis true,get_wallet_labels(&env)is invoked at line 96 (for the limit check) and again at line 111 (to push and persist). Each call performs a persistent storage read and conditionally bumps TTL. You can hoist the load and reuse the sameVecfor both the bound check and the push.♻️ Proposed refactor
- let is_new = !has_wallet(&env, &label); - - if is_new { - let labels = get_wallet_labels(&env); - if labels.len() >= MAX_WALLETS { - return Err(CoreError::WalletLimitReached); - } - } + let is_new = !has_wallet(&env, &label); + let mut labels = if is_new { + let existing = get_wallet_labels(&env); + if existing.len() >= MAX_WALLETS { + return Err(CoreError::WalletLimitReached); + } + Some(existing) + } else { + None + }; let entry = WalletEntry { label: label.clone(), address, chain, added_at: env.ledger().timestamp(), }; set_wallet(&env, &label, &entry); - if is_new { - let mut labels = get_wallet_labels(&env); - labels.push_back(label.clone()); - set_wallet_labels(&env, &labels); - } + if let Some(mut labels) = labels.take() { + labels.push_back(label.clone()); + set_wallet_labels(&env, &labels); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@onchain/contracts/core_contract/src/lib.rs` around lines 84 - 118, In add_wallet, avoid calling get_wallet_labels(&env) twice: when is_new is true, call let mut labels = get_wallet_labels(&env) once, perform the labels.len() >= MAX_WALLETS check against that same labels, and if allowed push_back the new label on that mut labels and then call set_wallet_labels(&env, &labels); remove the earlier separate get_wallet_labels call so get_wallet_labels is only invoked once per add_wallet execution.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@onchain/contracts/core_contract/src/lib.rs`:
- Around line 208-244: The create_escrow function allows release_at ≤ current
timestamp which breaks time-locking; add a check at the top of create_escrow
that compares release_at against env.ledger().timestamp() and returns
Err(CoreError::InvalidReleaseTime) when release_at is not strictly in the future
(<= now) before performing owner auth, token.transfer, or set_escrow; ensure an
InvalidReleaseTime variant exists in CoreError (add it in errors.rs if missing)
so callers receive a clear error.
- Around line 193-205: The code currently calls PeerCoreClient::resolve() which
uses the infallible client and will trap on peer-side errors; change to
PeerCoreClient::try_resolve() and propagate/map the inner contract error into
your CoreError (e.g., map_err(|e| CoreError::NoAddressLinked) or appropriate
variant) so send_to_username returns a domain error instead of aborting; update
the variable assignment (recipient) to handle the nested Result and use the
mapped Address on success before calling TokenClient::transfer and emit_send.
In `@onchain/contracts/escrow_contract/src/lib.rs`:
- Around line 46-47: The code treats missing contract setup state as domain
errors; add a new EscrowError variant (e.g., NotInitialized or
ContractNotInitialized) in onchain/contracts/escrow_contract/src/errors.rs with
a unique error code, then replace uses of .ok_or(EscrowError::Unauthorized) on
read_escrow_admin (function read_escrow_admin) and
.ok_or(EscrowError::CommitmentNotRegistered) on read_registration_contract
(function read_registration_contract) to return the new NotInitialized error
instead, and update any error handling/docs/tests accordingly so
contract-not-initialized paths are clearly distinguished from
authorization/commitment failures.
---
Nitpick comments:
In `@onchain/contracts/core_contract/src/lib.rs`:
- Around line 84-118: In add_wallet, avoid calling get_wallet_labels(&env)
twice: when is_new is true, call let mut labels = get_wallet_labels(&env) once,
perform the labels.len() >= MAX_WALLETS check against that same labels, and if
allowed push_back the new label on that mut labels and then call
set_wallet_labels(&env, &labels); remove the earlier separate get_wallet_labels
call so get_wallet_labels is only invoked once per add_wallet execution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 79f74bed-b407-432a-8b4e-7c85f102aed3
📒 Files selected for processing (5)
onchain/contracts/core_contract/src/lib.rsonchain/contracts/core_contract/src/storage.rsonchain/contracts/escrow_contract/src/lib.rsonchain/contracts/factory_contract/src/lib.rsonchain/shared/src/errors.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- onchain/shared/src/errors.rs
- onchain/contracts/factory_contract/src/lib.rs
AbuTuraab
force-pushed
the
feat/backend-resolver-api
branch
from
fb8126b to
734015c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
onchain/Cargo.toml (1)
12-12: 💤 Low valueSuppressing
missing_docs_in_private_itemsworkspace-wide is fine for contracts but worth noting.Private items in Soroban contracts don't form a public API, so silencing this lint has no user-facing risk. This is acceptable as-is, but if it was changed solely to accommodate a single crate, the same per-crate override pattern noted above applies here too.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@onchain/Cargo.toml` at line 12, The workspace Cargo.toml currently sets missing_docs_in_private_items = "allow" globally which suppresses the lint for all crates; instead, remove or revert this workspace-wide key and add the override only to the specific crate(s) that need it by placing missing_docs_in_private_items = "allow" in those crate-level Cargo.toml files (or remove it entirely if not strictly needed), so the suppression is scoped to the crate(s) requiring it rather than the whole workspace.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@onchain/Cargo.toml`:
- Line 13: Remove the workspace-wide unwrap_used = "allow" entry that disables
the unwrap_used lint for all crates; instead, add a crate-level lint override in
the auction_contract manifest to allow unwraps only for that crate (keeping the
lint enabled for core_contract, escrow_contract, factory_contract and shared).
Concretely: delete the unwrap_used = "allow" line from the workspace Cargo.toml
and add an equivalent allow override under the auction_contract crate's
[package.metadata] lint settings (or appropriate per-crate lint section) so only
auction_contract has unwrap_allowed while all other crates retain the lint.
In `@onchain/contracts/auction_contract/src/test.rs`:
- Around line 59-69: In the event loop inside tests
test_bid_refunded_event_emitted_when_outbid and test_bid_placed_event_emitted,
avoid calling topics.get(0).expect(...) which panics on zero-topic events;
instead guard with if let Some(first_topic) = topics.get(0) { let event_name =
soroban_sdk::Symbol::try_from_val(&env, first_topic); ... } so zero-topic events
are skipped and the loop can finish to hit assert!(found, ...); apply this
change where topics.get(0).expect("event topic missing") and the subsequent
Symbol::try_from_val calls appear (both tests).
---
Nitpick comments:
In `@onchain/Cargo.toml`:
- Line 12: The workspace Cargo.toml currently sets missing_docs_in_private_items
= "allow" globally which suppresses the lint for all crates; instead, remove or
revert this workspace-wide key and add the override only to the specific
crate(s) that need it by placing missing_docs_in_private_items = "allow" in
those crate-level Cargo.toml files (or remove it entirely if not strictly
needed), so the suppression is scoped to the crate(s) requiring it rather than
the whole workspace.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 80ca35e4-c2e8-4dd9-9cda-2355c04c9a0a
⛔ Files ignored due to path filters (1)
onchain/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.github/workflows/pre-commit-validation.ymlonchain/Cargo.tomlonchain/contracts/auction_contract/src/test.rsonchain/contracts/core_contract/src/lib.rsonchain/contracts/core_contract/src/test.rsonchain/contracts/escrow_contract/src/lib.rsonchain/contracts/factory_contract/src/types.rs
✅ Files skipped from review due to trivial changes (1)
- onchain/contracts/core_contract/src/test.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- onchain/contracts/escrow_contract/src/lib.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #397
Implemented the username resolver API for resolving username hashes to linked addresses.
Changes include:
Added ResolverModule, ResolverController, and ResolverService
Added GET /resolve/:usernameHash for full address resolution
Added GET /resolve/:usernameHash/stellar for Stellar-only resolution
Added UsernameHashDto validation with class-validator
Added cache-first lookup against the Username entity
Added fallback contract resolution via resolve_stellar
Added clear 404 handling when a username hash is not found on-chain
Added unit tests for cache hit, cache miss, and not-found paths
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests