refactor: extract shared authorization and storage helpers across onc…#424
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR adds shared utility functions to the shared module for Option handling, timestamp validation, and instance-storage counter management. It also refactors the bid-validation control flow in the auction contract to improve readability without changing behavior. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
|
Please change you branch to dev from main |
bamiebot-maker
force-pushed
the
refactor/extract-shared-onchain-helpers
branch
from
3687c4c to
ba7c14e
Compare
|
done,pr keep getting conflicts i am tired asf |
|
resolved |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 35
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (25)
onchain/contracts/core_contract/src/types.rs (1)
32-59:⚠️ Potential issue | 🔴 CriticalResolve this merge conflict by unioning both sides — accepting only HEAD will delete
PermissionandPermissionSet, breaking the codebase.The two branches define incompatible content at the same location:
- HEAD: documentation comments +
pub type Proof = Bytes- upstream/dev:
Permissionenum,PermissionSetstruct, andpub type Proof = BytesNaively accepting HEAD deletes
PermissionandPermissionSet, which are referenced throughout the codebase: instorage.rs(function signatures for delegate permissions),registration.rs(grant_delegate),resolver.rs(permission checks),address_manager.rs(address management permissions),lib.rs(contract interface), andtest.rs(test cases). The compiler will fail with missing type errors.The correct resolution is to keep HEAD's documentation, upstream/dev's type definitions, and a single
pub type Proof = Bytes(no duplication).🛠️ Proposed resolution (union both sides)
-<<<<<<< HEAD +#[contracttype] +#[derive(Clone, Debug, Eq, PartialEq)] +pub enum Permission { + SetMemo = 1, + SetPrivacyMode = 2, + AddChainAddress = 3, + RemoveChainAddress = 4, + AddStellarAddress = 5, + RemoveStellarAddress = 6, +} + +#[contracttype] +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct PermissionSet { + pub permissions: soroban_sdk::Vec<Permission>, +} + /// Serialized Groth16 proof bytes submitted by the caller. pub type Proof = Bytes; /// Public signals extracted from a Groth16 non-inclusion proof. /// `old_root` must match the current on-chain SMT root. /// `new_root` becomes the new SMT root after a successful registration. -======= -#[contracttype] -#[derive(Clone, Debug, Eq, PartialEq)] -pub enum Permission { - SetMemo = 1, - SetPrivacyMode = 2, - AddChainAddress = 3, - RemoveChainAddress = 4, - AddStellarAddress = 5, - RemoveStellarAddress = 6, -} - -#[contracttype] -#[derive(Clone, Debug, Eq, PartialEq)] -pub struct PermissionSet { - pub permissions: soroban_sdk::Vec<Permission>, -} - -pub type Proof = Bytes; - ->>>>>>> upstream/dev #[contracttype] #[derive(Clone)] pub struct PublicSignals {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/core_contract/src/types.rs` around lines 32 - 59, The merge conflict removed the Permission and PermissionSet definitions; restore them and keep the HEAD doc comment for Proof so all types exist exactly once: reintroduce the upstream/dev enum Permission and struct PermissionSet (with #[contracttype] and derives) above the existing doc comment and retain the single pub type Proof = Bytes from HEAD (remove duplicate Proof if present), ensuring symbols Permission, PermissionSet, and Proof are present for use by storage.rs, registration.rs, resolver.rs, address_manager.rs, lib.rs, and tests.onchain/contracts/escrow_contract/src/events.rs (1)
3-224:⚠️ Potential issue | 🔴 CriticalCritical: nine unresolved merge-conflict marker blocks throughout the file.
Conflict markers appear at lines 3-6, 18-21, 32-35, 47-50, 62-65, 74-77, 87-90, 100-103, 111-114, 146-149, and 218-221. In every case, the only difference between sides is added doc comments on HEAD vs. no comments on
upstream/dev. The file cannot compile until these are resolved.Recommend keeping the HEAD side (the doc comments) since the PR’s stated goal is to improve readability without changing behavior — none of the event field signatures or
publishcalls differ between the two sides.🛠️ Resolution pattern (apply uniformly to all 9 blocks)
-<<<<<<< HEAD /// Event emitted when ... -======= ->>>>>>> upstream/dev #[contractevent]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/escrow_contract/src/events.rs` around lines 3 - 224, The file contains unresolved Git conflict markers (<<<<<<<, =======, >>>>>>>) around multiple doc-comment blocks; remove all conflict marker lines and keep the HEAD-side doc comments so the event structs and Events methods compile cleanly—apply this to the blocks around SchedulePayEvent, PayExecEvent, AutoSetEvent, AutoPayEvent, VaultCancelEvent, DepositEvent, WithdrawEvent, AutoCancelEvent and the Events helper methods (including vault_crt and auto_cancel), ensuring only the doc comments remain and no duplicate or marker lines are left.onchain/contracts/core_contract/src/events.rs (1)
10-11:⚠️ Potential issue | 🟡 MinorRemove unused constant
ADDR_ADDED.The constant
ADDR_ADDEDis defined but never used. BothADDR_ADDEDandADDR_ADDexpand to the same on-chain symbol"ADDR_ADD", but onlyADDR_ADDis imported and emitted in address_manager.rs. Remove the duplicateADDR_ADDEDconstant at line 10.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/core_contract/src/events.rs` around lines 10 - 11, Remove the duplicate, unused constant declaration ADDR_ADDED that expands to the same symbol as ADDR_ADD; delete the line defining ADDR_ADDED and keep the existing ADDR_ADD constant (symbol_short!("ADDR_ADD")), and run a quick search to confirm no remaining references to ADDR_ADDED (update imports/usages if any are found).onchain/contracts/shared/Cargo.toml (1)
1-13:⚠️ Potential issue | 🔴 CriticalCritical: Resolve git merge conflict in Cargo.toml — file contains unresolved
<<<<<<< HEAD/=======/>>>>>>> upstream/devmarkers.The TOML parse error (
key with no value, expected '=') occurs because merge conflict markers like<<<<<<< HEADare being parsed as malformed TOML keys. The file cannot be parsed until the conflict is resolved. Once resolved, the configuration is sound:soroban-sdkis properly defined at the workspace level andsharedis registered as a workspace member.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/shared/Cargo.toml` around lines 1 - 13, The Cargo.toml contains unresolved git merge markers (<<<<<<<, =======, >>>>>>>) that break TOML parsing; open the Cargo.toml and remove the conflict markers, keeping the valid TOML entries such as the [package] table (name = "shared", version, edition, publish = false), the [lib] table (crate-type and doctest), and the [dependencies] entry soroban-sdk = { workspace = true }; ensure there are no stray conflict lines and that TOML syntax (key = "value" and table headers) is preserved, and confirm the workspace includes the "shared" member in your workspace configuration if applicable.onchain/contracts/factory_contract/src/test.rs (1)
18-33:⚠️ Potential issue | 🟠 MajorPreserve the dev-side API and tests — they represent a superior design that HEAD inadvertently discards.
The merge conflict in this file masks two critical architectural decisions:
initialize()method is part of the intended API (dev side): The dev branch introduces a two-phase initialization pattern:initialize(owner)sets up ownership/access control, thenconfigure(auction, core)links contract dependencies with operator-level auth. HEAD removes the entireinitializemethod. This breakssetup_factoryandsetup_unconfigured_factory, both of which callfactory.initialize(&owner)on the dev side. The dev pattern is architecturally superior—it properly separates concerns and enforces authorization at each phase.
test_transfer_username_*suite must be preserved (dev side only): Lines 386–500 contain four distinct tests covering thetransfer_usernameentrypoint: success case, unconfigured-auction failure, missing-record failure, and unauthorized-caller failure. These tests exist only on the dev side and are completely absent in HEAD. Discarding them is a test-coverage regression and violates any requirement to maintain or increase test coverage for public APIs.Resolution: Resolve both conflicts in favor of
upstream/dev. Retaininitialize(), the two-phase setup insetup_factory/setup_unconfigured_factory, and all fourtest_transfer_username_*functions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/factory_contract/src/test.rs` around lines 18 - 33, The merge conflict removed the dev-side two-phase initialization and tests; restore the upstream/dev changes by keeping FactoryContract::initialize(owner) and the two-phase setup used by setup_factory and setup_unconfigured_factory (including env.mock_all_auths() and owner generation), and re-add the four test functions named test_transfer_username_success, test_transfer_username_unconfigured_auction, test_transfer_username_missing_record, and test_transfer_username_unauthorized_caller so the transfer_username behavior and auth/configuration tests are preserved; ensure FactoryContractClient::initialize(&owner) is invoked where the dev branch did so and that configure(auction, core) remains for operator-level linking.onchain/contracts/core_contract/src/admin.rs (1)
1-150:⚠️ Potential issue | 🔴 CriticalUnresolved merge conflicts — and the two sides choose different access-control models.
Conflicts at lines 1–12 (imports), 18–50 (
initialize), and 55–150 (the whole admin API surface). The behavioral divergence is significant:
Concern HEAD upstream/dev initializesets only ownerviashared_auth::require_address_authalso sets admin = owner,operator = ownerupdate_smt_rootgated on owner gated on operator Extra entrypoints none get_admin,get_operator,set_admin(owner-only),set_operator(admin-only), withROLE_GRANTEDeventsPicking HEAD silently removes the RBAC role API from the public contract surface — a breaking change for any caller that already integrates against the dev branch. Picking upstream/dev keeps RBAC but discards the
shared_authadoption that is the entire point of this refactor PR.A reasonable middle ground: keep the RBAC entrypoints (upstream/dev) but rewrite their auth/storage paths through
shared_auth::require_address_authandshared_auth::unwrap_or_panicso the refactor goal is still achieved without dropping role functionality. Whatever you choose, this file,address_manager.rs,events.rs(forROLE_GRANTED), andtest.rsmust all be aligned to one API.Note: the post-marker
RootUnchangedshort-circuit at lines 152–160 reads viaenv.storage().instance()...directly rather than throughshared_storage::get_instance— worth threading through the shared helper for consistency once the conflict is resolved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/core_contract/src/admin.rs` around lines 1 - 150, Resolve the merge by keeping the RBAC API from upstream/dev (retain functions initialize, get_contract_owner, get_admin, get_operator, set_admin, set_operator, update_smt_root and the ROLE_GRANTED events) but rewire auth/storage calls to use the refactor's shared_auth helpers: in initialize call shared_auth::require_address_auth(&owner) then storage::set_owner/set_admin/set_operator(&env,&owner); change get_* getters to use shared_auth::unwrap_or_panic(&env, storage::get_*(&env), CoreError::NotFound); in set_admin/set_operator require the caller via shared_auth::require_address_auth on owner/admin respectively before calling storage::set_admin/set_operator and publishing ROLE_GRANTED (use symbol_short! for role symbols); and in update_smt_root require the operator via shared_auth::require_address_auth(&operator) after fetching operator with shared_auth::unwrap_or_panic or equivalent; finally ensure get_smt_root uses smt_root::SmtRoot::get_root(...).unwrap_or_else(|| panic_with_error!(... CoreError::RootNotSet)) so behavior and shared_auth usage are consistent.onchain/contracts/factory_contract/src/events.rs (1)
3-43:⚠️ Potential issue | 🔴 CriticalResolve merge conflict in events.rs constants and doc comments.
Lines 3–43 contain unresolved merge markers (
<<<<<<<,=======,>>>>>>>). The conflict has two parts:
- Constants block (lines 3–22): HEAD omits
ROLE_GRANTED; upstream/dev includes it.- Doc comments (lines 35–43): Minor wording differences ("Event topic emitted" vs. "Event emitted").
ROLE_GRANTED is actively used: factory_contract/src/lib.rs imports
ROLE_GRANTED(line 39) and publishes it inset_adminandset_operator(lines 132, 143). If HEAD is chosen, those imports and usages will break. If upstream/dev is chosen, the constant is already consumed by the same crate's code.Choose and apply one side consistently:
- upstream/dev: Keeps ROLE_GRANTED and aligns with the existing RBAC calls in lib.rs.
- HEAD: Requires removing ROLE_GRANTED constant and removing its imports/usages from lib.rs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/factory_contract/src/events.rs` around lines 3 - 43, Remove the merge markers and adopt the upstream/dev changes: keep the ROLE_GRANTED constant (pub const ROLE_GRANTED: Symbol = symbol_short!("ROLE_GNT");), unify the doc comments to the upstream/dev wording, and ensure the constants USERNAME_DEPLOYED and OWNERSHIP_TRANSFERRED remain as in upstream/dev; also remove any duplicated or conflicting comment blocks around emit_username_deployed and the subsequent ownership-event comment so the file has a single clean set of constants and doc comments and no conflict markers.onchain/contracts/auction_contract/src/indexed.rs (1)
1-200:⚠️ Potential issue | 🔴 CriticalResolve merge conflicts across three files with misaligned error-handling contracts.
Merge conflict markers are present in
storage.rs(17 blocks),lib.rs(11 blocks), andindexed.rs(6 blocks). The conflicts introduce two incompatible error-handling paradigms:
- HEAD path: panic-based (functions return
(), usepanic_with_error!). Storage helpers returnAddressdirectly.- upstream/dev path: Result-based (functions return
Result<(), AuctionError>, propagate with?). Storage helpers returnResult<Address, AuctionError>.Removing conflict markers without aligning the type signatures across all three files will cause compilation failures. Ensure
storage.rs,lib.rs, andindexed.rsall adopt the same error-handling strategy consistently.Secondary issue: the reuse of
AuctionNotOpenfor "auction already exists" (create_auction, line 21) andAuctionNotClosedfor "end_time in the past" (line 24) is misleading — consider dedicated error variants.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/auction_contract/src/indexed.rs` around lines 1 - 200, Resolve the merge markers and make error handling consistent across storage.rs, lib.rs, and indexed.rs by adopting the Result-based paradigm: change indexed.rs functions create_auction, place_bid, close_auction_by_id, and claim to return Result<(), errors::AuctionError>, remove panic_with_error! calls and replace them with returning Err(...), and call storage helpers with ? (which requires making storage helper signatures return Result<T, AuctionError> in storage.rs); ensure lib.rs entrypoints match the Result signatures too. Also replace misleading error variants used in create_auction (don’t reuse AuctionNotOpen/AuctionNotClosed) by adding/using clearer variants like AuctionAlreadyExists and EndTimeInPast in errors::AuctionError and propagate those where appropriate.onchain/contracts/core_contract/src/test.rs (1)
3-996:⚠️ Potential issue | 🔴 CriticalUnresolved merge conflicts prevent compilation — the test file must be aligned to a single contract API.
The file contains approximately 45 merge conflict regions (135 conflict markers) that span incompatible APIs. This is a blocker: unresolved merge markers are syntax errors in Rust, so the file cannot compile and no tests could have executed.
The two sides encode fundamentally different contract designs:
- HEAD: owner-only auth model. Function calls like
client.set_memo(&hash, &v),client.set_privacy_mode(&hash, mode). No imports ofPermissionorPermissionSet. Test functions focused on owner-based authorization.- upstream/dev: RBAC (admin/operator roles) plus delegate permission model. Function calls like
client.set_memo(&caller, &hash, &v),client.set_privacy_mode(&owner, &hash, mode)— owner is now a required parameter. ImportsPermissionandPermissionSet. Tests includetest_rbac_roles,test_delegate_scoped_permissions,test_delegate_address_management,test_unauthorized_delegate_rejected.Example conflict (lines ~445–448):
- HEAD:
client.set_privacy_mode(&hash, &PrivacyMode::Shielded);- upstream/dev:
client.set_privacy_mode(&owner, &hash, &PrivacyMode::Shielded);Beyond these 45 regions, the foundational decision — owner-only vs. RBAC+delegate — has cascading effects across
admin.rs,lib.rs, and the contract client interface.Choose one API direction, resolve every conflict marker, run
cargo test, and confirm all tests pass before merge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/core_contract/src/test.rs` around lines 3 - 996, This file contains unresolved Git merge markers and two incompatible APIs (owner-only vs RBAC/delegate)—remove all conflict markers and pick one API surface (either HEAD owner-only or upstream/dev RBAC) then consistently apply it across imports and tests: unify imports (include or remove Permission/PermissionSet), align client method signatures and calls (e.g., choose between client.set_memo(&hash, ..) vs client.set_memo(&caller, &hash, ..); client.set_privacy_mode(&hash, ..) vs client.set_privacy_mode(&owner, &hash, ..); client.initialize(&owner) and RBAC helpers like client.set_admin, client.set_operator, client.update_smt_root), fix related helper/tests (test_rbac_roles, test_delegate_* or owner-only tests) to match the chosen API, delete all <<<<<<<, =======, >>>>>>> markers, run cargo test and iterate until compilation/tests pass.onchain/contracts/auction_contract/src/test.rs (1)
19-209:⚠️ Potential issue | 🔴 CriticalResolve all 17 merge conflict markers — and align event symbol assertions with the chosen emission architecture.
The file contains 17 unresolved conflict markers (lines 19–23, 42–64, 85–89, 107–110, 123–133, 138–142, 148–169, 178–181, 189–202, plus decorative comment regions). More critically, the two conflict branches assert incompatible event names because
events.rsitself contains unresolved conflicts in theemit_bid_placedandemit_bid_refundedfunctions:
Test HEAD asserts upstream/dev asserts test_bid_refunded_event_emitted_when_outbidsymbol "BID_RFDN"(short)symbol "bid_refunded_event"(derived)test_bid_placed_event_emittedsymbol "BID_PLCD"(short)symbol "bid_placed_event"(derived)HEAD uses deprecated
env.events().publish()withsymbol_short!()constants; upstream/dev uses the#[contractevent]macro (which auto-derives snake_case symbol names). These are mutually exclusive architectural approaches. Pick one branch, resolve all conflict markers in bothevents.rsandtest.rs, and ensure test symbol assertions match the emitted symbol names.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/auction_contract/src/test.rs` around lines 19 - 209, Resolve all merge conflict markers left in the file and pick one event-emission architecture (either the legacy publish with symbol_short! constants or the #[contractevent] macro). Concretely: remove the <<<<<<</=======/>>>>>>> blocks throughout and keep a single consistent implementation for emit_bid_placed and emit_bid_refunded (or their equivalents), then update the tests test_bid_refunded_event_emitted_when_outbid and test_bid_placed_event_emitted so their Symbol assertions match the chosen emitted symbol (e.g., if you keep symbol_short! names use "BID_RFDN"/"BID_PLCD"; if you keep #[contractevent] use "bid_refunded_event"/"bid_placed_event"). Ensure tests inspect topics.get(0) and compare with soroban_sdk::Symbol::new(&env, "<chosen_name>") and remove the alternative decoding branches so only one consistent check remains.onchain/contracts/core_contract/src/address_manager.rs (1)
1-555:⚠️ Potential issue | 🔴 CriticalUnresolved merge conflicts — HEAD's authentication approach will break delegate-based address management tests.
Conflicts at lines 1–4, 10–16, 27–48, 56–79, 86–95, 102–117, 124–149, 156–184, 193–212, 219–267, 273–294, 301–335, 343–359, 364–370, 375–382, 397–429, 435–465, 467, 474–488, 497–511, 516–529, 534–544.
The behavioral divergence is critical:
- HEAD routes
add_chain_address,remove_chain_address,add_stellar_address, andremove_stellar_addressthroughshared_auth::require_matching_auth(&env, &caller, &owner, …). This function only succeeds whencaller == owner; any delegate is rejected outright.- upstream/dev keeps the pattern:
if owner != caller && !storage::has_permission(...), which allows an authorized delegate to proceed.The test
test_delegate_address_managementexplicitly grants a delegatePermission::AddChainAddress,Permission::RemoveChainAddress,Permission::AddStellarAddress, andPermission::RemoveStellarAddress, then calls those methods via the delegate. Choosing HEAD will cause this test to fail.Resolution: Keep the delegate fallback pattern from upstream/dev to honor granted permissions. The error enum bounds (
ChainAddressErrorandCoreErrorboth implementCopy + Into<Error>) are compatible withrequire_matching_auth, so the issue is the logic, not the types.Note on
add_shielded_address: Both branches enforcecaller == ownerwith no delegate path. There is noPermission::AddShieldedAddressvariant in the enum, so shielded-address management is consistently owner-only in both branches. If shielded privacy commitments should support delegation, add the permission type and fallback explicitly; otherwise, the current behavior is consistent.🤖 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 1 - 555, Merge conflict left HEAD's strict owner-only auth; restore upstream/dev's delegate-aware behavior for address management: in add_chain_address, remove_chain_address, add_stellar_address, and remove_stellar_address use caller.require_auth(), read owner via env.storage().persistent().get or Registration::get_owner().unwrap_or_else(...), then allow operation when owner == caller OR storage::has_permission(&env, &username_hash, &caller, Permission::...) otherwise panic_with_error!(..., Unauthorized); persist addresses via env.storage().persistent().set/remove and for chain addresses extend_ttl with PERSISTENT_LIFETIME_THRESHOLD and PERSISTENT_BUMP_AMOUNT; remove shared_auth/shared_storage usage and stash the upstream imports (storage::{self, PERSISTENT_BUMP_AMOUNT, PERSISTENT_LIFETIME_THRESHOLD} and types::{ChainType, Permission}); keep add_shielded_address owner-only as-is (no delegate permission) and eliminate all merge markers.onchain/contracts/factory_contract/src/types.rs (1)
1-79:⚠️ Potential issue | 🔴 CriticalCritical: unresolved merge conflict markers — file will not compile.
Conflicts at lines 3‑33, 39‑59, 65‑77 must be resolved. The HEAD
DataKeydropsOwner,Admin,Operatorvariants; this must stay consistent with whichever side wins infactory_contract/src/lib.rsandfactory_contract/src/storage.rs(see those reviews — keeping HEAD here forces deletion of the role management entrypoints).The
UsernameRecordandDeployConfigstruct shapes are identical on both sides; only the doc comments differ. Once resolved, simply keep the HEAD doc strings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/factory_contract/src/types.rs` around lines 1 - 79, Remove the unresolved merge markers and restore a single coherent definition for DataKey/UsernameRecord/DeployConfig: delete the <<<<<<<, =======, >>>>>>> lines and choose the HEAD variant of the doc comments for UsernameRecord and DeployConfig, but for DataKey ensure the enum includes or omits Owner/Admin/Operator to match the role-management used in factory_contract/src/lib.rs and factory_contract/src/storage.rs (if those modules reference Owner/Admin/Operator keep those variants; otherwise keep the simpler HEAD set with AuctionContract/CoreContract/Username/Config), then recompile to confirm consistency across the three files and update any code in lib.rs/storage.rs to match the final DataKey shape if needed.onchain/contracts/auction_contract/src/lib.rs (2)
1-300:⚠️ Potential issue | 🔴 CriticalCritical: unresolved merge conflict markers — file will not compile.
Conflicts at lines 2‑5, 15‑19, 24‑29, 42‑58, 67‑70, 84‑91, 99‑143, 150‑190, 195‑220, 228‑265, 272‑284 must be resolved.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/auction_contract/src/lib.rs` around lines 1 - 300, The file contains unresolved git conflict markers (<<<<<<<, =======, >>>>>>>) — remove those markers and reconcile the two versions: keep the consolidated, compiling implementations (use the Result-returning require_status signature or refactor callers to match), ensure imports are consistent (remove or use shared::auth vs Address::require_auth), and apply the upstream/dev changes for indexed flow functions (create_auction, place_bid, refund_bid, close_auction_by_id, claim, get_auction_info) so their signatures return Result<(), errors::AuctionError> and use storage functions that return Result where required (e.g., storage::auction_get_asset(&env)?). Also ensure hash-indexed APIs (get_auction, has_auction, get_bid, get_all_bidders) remain in the final impl block and remove duplicate/merged impl annotations so the crate compiles.
99-202:⚠️ Potential issue | 🟠 MajorBehavioral change: ID-scoped entrypoints become panic-only, breaking the contract interface.
The HEAD path converts
create_auction,place_bid,refund_bid,close_auction_by_id, andclaimfrom returningResult<(), AuctionError>to returning()with error handling viapanic_with_error!. This is a breaking change to the contract's external interface—callers that previously inspected error results will now host-trap on error instead.The type-checking concern is valid but resolvable: the indexed module functions (
indexed::create_auction,indexed::place_bid,indexed::close_auction_by_id,indexed::claim) also return()on HEAD, so the calls at lines 101, etc., do type-check correctly.However, this conversion conflicts with the stated objective to "preserve external contract interfaces unless other issues require changes." Document this breaking change in the PR description and bump the contract version if intentional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/auction_contract/src/lib.rs` around lines 99 - 202, The PR inadvertently changed several external, ID-scoped entrypoints (create_auction, place_bid, refund_bid, close_auction_by_id, claim) from returning Result<(), errors::AuctionError> to panicking ()-only signatures, breaking the contract interface; restore these functions to return Result<(), errors::AuctionError> (match the previous signatures for create_auction, place_bid, refund_bid, close_auction_by_id, claim), propagate errors by returning Err(...) instead of calling soroban_sdk::panic_with_error!, and keep indexed::* callers consistent with Result returns (or wrap their panics into Err where needed); if the panic-to-Result change was intentional, document it in the PR description and bump the contract version to reflect the breaking API change.onchain/contracts/core_contract/src/resolver.rs (1)
1-226:⚠️ Potential issue | 🔴 CriticalCritical: unresolved merge conflict markers — file will not compile.
Conflict regions at lines 7‑11, 17‑41, 83‑101, 108‑119, 126‑163, 172‑185, 190‑208 must be resolved.
Reconciliation note (must be coordinated with
core_contract/src/storage.rs): the two sides take opposite design directions for authorization onset_memoandset_privacy_mode:
- HEAD removes the
caller: Addressparameter and thePermission::SetMemo/Permission::SetPrivacyModepermission paths entirely;set_privacy_modefalls back toowner.require_auth()andset_memoperforms no auth at all.- upstream/dev requires
caller.require_auth()plus anowner == caller || has_permission(...)delegate check.Two concerns with picking HEAD as-is:
set_memohas no authorization check on the HEAD path (lines 96 → 124 once conflicts are removed). Anyone can overwrite the memo for any registered commitment. This is a critical authorization regression — at minimum the owner of the commitment shouldrequire_auth().- The
Permissionimport at line 10 is only used by the upstream/dev code paths; if HEAD wins it must be dropped fromcrate::types, and the correspondinghas_permission/Delegatemachinery instorage.rsis being deleted along with it (see review oncore_contract/src/storage.rs).Please pick a single auth model and apply it consistently across both files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/core_contract/src/resolver.rs` around lines 1 - 226, Resolve the merge-conflict markers and adopt the upstream/dev authorization model: keep the caller: Address parameter on set_memo and set_privacy_mode, ensure caller.require_auth() is called, fetch owner via Registration::get_owner, and enforce owner == caller || storage::has_permission(&env, &commitment/username_hash, &caller, Permission::SetMemo/Permission::SetPrivacyMode) (panicking with CoreError::Unauthorized on failure); restore the Permission import from crate::types and ensure storage::has_permission exists and is used consistently; remove duplicate/conflicting doc blocks and leftover conflict markers so the file compiles.onchain/contracts/core_contract/src/storage.rs (1)
1-222:⚠️ Potential issue | 🔴 CriticalCritical: unresolved merge conflict markers — file will not compile.
The entire file is bracketed by
<<<<<<< HEAD/=======/>>>>>>> upstream/devmarkers (lines 1‑4, 9‑36/36‑59, 64‑73, 83‑119, 128‑147, 158‑221).cargo buildandcargo clippycannot succeed in this state, so the issue's acceptance criteria ("cargo testandcargo clippypass with no warnings") cannot be met until these are resolved.Beyond the mechanical resolution, please be aware of the semantic regression if the HEAD side is chosen verbatim:
- The HEAD
DataKeyenum drops theAdmin,Operator, andDelegate(BytesN<32>, Address)variants.- The HEAD branch deletes
set_admin/get_admin,set_operator/get_operator,set_delegate_permissions/get_delegate_permissions/remove_delegate_permissions, andhas_permission.This is more than a refactor — it's a deletion of the contract's role/permission infrastructure, which contradicts the linked issue's "preserve external contract interfaces unless other issues require changes" requirement. If admin/operator/delegate functionality is intended to remain, you must merge the upstream/dev variants and helpers back in (and route their reads/writes through
shared_storage::set_instance/set_persistentto keep the refactor benefit).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/core_contract/src/storage.rs` around lines 1 - 222, Remove all merge conflict markers and fully merge the upstream/dev changes: restore the DataKey variants Admin, Operator, and Delegate(BytesN<32>, Address) and all helper functions set_admin/get_admin, set_operator/get_operator, set_delegate_permissions/get_delegate_permissions/remove_delegate_permissions/has_permission so role/permission behavior is preserved; ensure these functions use the project's shared storage helpers (shared_storage::set_instance/shared_storage::get_instance for instance keys and shared_storage::set_persistent/shared_storage::get_persistent for persistent keys) and that persistent writes also extend TTL consistent with PERSISTENT_BUMP_AMOUNT and PERSISTENT_LIFETIME_THRESHOLD (e.g., when replacing env.storage().persistent().set/extend_ttl with shared_storage calls, include TTL extension semantics).onchain/contracts/factory_contract/src/lib.rs (1)
1-327:⚠️ Potential issue | 🔴 CriticalCritical: unresolved merge conflict markers + external interface regression.
Conflict regions at lines 2‑20, 26‑45, 53‑88, 90‑175, 191‑220, 227‑249, 254‑277, 282‑294, 299‑311, 316‑326 must be resolved before this can build.
If the HEAD side is taken as-is, the following public entrypoints disappear, which is a breaking change to the factory's external interface:
initialize(env, owner)set_admin(env, new_admin)/set_operator(env, new_operator)get_owner/get_admin/get_operatorThe PR description states "Preserve external contract interfaces unless other issues require changes" — removing these entrypoints conflicts with that. Either keep them (routing through
shared_auth/shared_storage) or document the intentional removal in the PR description and bump the contract version.Additionally, on the HEAD branch (lines 203‑206),
transfer_usernamepanics via.expect("Username not deployed")instead ofpanic_with_error!(&env, FactoryError::…)likedeploy_usernamedoes at line 115. This loses the typedFactoryErrorcode on-chain and is inconsistent with the rest of the contract's panic style.🛠️ Suggested fix for `transfer_username` panic style
- let mut record = get_username(&env, &username_hash).expect("Username not deployed"); + let mut record = get_username(&env, &username_hash) + .unwrap_or_else(|| panic_with_error!(&env, FactoryError::NotDeployed));(Use whichever existing
FactoryErrorvariant matches "username not registered"; add one if absent.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/factory_contract/src/lib.rs` around lines 1 - 327, The file contains unresolved merge conflict markers and a broken external interface: remove all conflict markers and merge the intended changes so the public entrypoints initialize, set_admin, set_operator, get_owner, get_admin, and get_operator remain exported (or explicitly document/version-change) by keeping their implementations (symbols: initialize, set_admin, set_operator, get_owner, get_admin, get_operator) and keep configure/deploy_username/transfer_username logic consistent; replace the HEAD-style require_auction_contract_auth usage with the upstream pattern only if you also restore the read_auction_contract path and callers, and in transfer_username replace the .expect("Username not deployed") with panic_with_error!(&env, FactoryError::SomethingNotRegistered) (or the appropriate existing FactoryError variant) so errors use typed FactoryError like deploy_username; ensure storage helpers referenced (get_owner/get_admin/get_operator/set_admin/set_operator/set_owner/read_auction_contract/read_core_contract/set_auction_contract/set_core_contract/get_username/has_username/set_username) are used consistently after resolving conflicts.onchain/contracts/auction_contract/src/storage.rs (1)
1-457:⚠️ Potential issue | 🔴 CriticalCritical: unresolved merge conflict markers — file will not compile.
Conflicts at lines 1‑12 / 12‑20, 31‑127, 143‑166, 171‑194, 199‑208, 220‑229, 241‑250, 261‑270, 282‑291, 303‑312, 324‑333, 345‑354, 366‑381, 392‑401, 411‑417, 422‑431, 446‑455 must be resolved.
Two important semantic issues if HEAD is taken as-is:
auction_get_sellerandauction_get_assetchange return type fromResult<Address, AuctionError>toAddressandexpect(...)onNone(lines 147‑151, 175‑179). This is a breaking change to internal API and turns recoverable error states (AuctionError::InvalidState) into raw panics with a string message — losing the typed contract error code. Callers inlib.rs(e.g.,refund_bidline 166,get_auction_infoline 243‑244) will silently lose the ability to surfaceAuctionError::InvalidState. If you intend to panic, preferpanic_with_error!(&env, AuctionError::InvalidState)for consistency with the rest of the contract.
use crate::errors::AuctionError;is removed in HEAD (line 13 vs 2‑12). If other functions in this file or downstream still need it, this will not compile post-resolution; please double-check after picking sides.
use crate::types::AuctionKey;at line 128 is mid-file, just after a conflict region. It should live at the top with the otherusestatements (line 4). It compiles in Rust but is unusual and looks like a merge artifact.🛠️ Suggested adjustments for HEAD path
-pub fn auction_get_seller(env: &Env, id: u32) -> Address { - env.storage() - .persistent() - .get(&AuctionKey::Seller(id)) - .expect("seller must be set before auction close") +pub fn auction_get_seller(env: &Env, id: u32) -> Address { + env.storage() + .persistent() + .get(&AuctionKey::Seller(id)) + .unwrap_or_else(|| soroban_sdk::panic_with_error!(env, AuctionError::InvalidState)) }(Apply analogously to
auction_get_asset. Re-adduse crate::errors::AuctionError;at the top of the file.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/auction_contract/src/storage.rs` around lines 1 - 457, Remove all unresolved git conflict markers and keep the upstream/dev changes: use env.storage() persistent APIs with TTL bumps (not shared_storage calls), restore the Result return signatures for auction_get_seller and auction_get_asset (return Result<Address, AuctionError>) and use AuctionError for missing values, re-add use crate::errors::AuctionError; to the top-level use block, relocate use crate::types::AuctionKey; to the top with other use statements, and ensure functions that set persistent values (e.g., auction_set_status, auction_set_seller, auction_set_asset, auction_set_min_bid, auction_set_end_time, auction_set_highest_bidder, auction_set_highest_bid, auction_set_claimed, auction_set_username_hash, auction_set_outbid_amount, auction_set_bid_refunded, set_auction, set_bid, add_bidder) call env.storage().persistent().set(...) and extend_ttl(...) as in upstream/dev so the file compiles and preserves typed error handling.onchain/contracts/escrow_contract/src/types.rs (1)
1-160:⚠️ Potential issue | 🔴 CriticalCritical: unresolved merge conflict markers — file will not compile.
Merge conflicts at lines 3–122 and 133–159 must be resolved. The two sides are semantically equivalent — only doc comments and struct field documentation differ — so resolution is straightforward: keep the HEAD doc-commented variants and remove the upstream/dev block.
Update doc comment on
LegacyVault(lines 66–81): The snapshot fixture paths referencegateway-contract/contracts/escrow_contract/test_snapshots/..., which no longer exist after the directory structure migration. However, these snapshot files don't appear to exist anywhere in the current repository—neither under the old paths nor underonchain/contracts/escrow_contract/test_snapshots/. Either clarify whether snapshots are still maintained (and update paths accordingly), or remove the outdated snapshot reference from the migration documentation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/escrow_contract/src/types.rs` around lines 1 - 160, Remove the unresolved merge conflict markers and duplicate blocks so the file compiles: keep the HEAD/annotated versions of DataKey, VaultConfig, VaultState, ScheduledPayment, LegacyVault and AutoPay (the variants with doc comments and field docs), delete the upstream/dev duplicate enums/structs and all <<<<<<<, =======, >>>>>>> markers, and ensure there is only one definition of each type (DataKey, VaultConfig, VaultState, ScheduledPayment, LegacyVault, AutoPay). While editing LegacyVault's migration doc comment, either remove the outdated snapshot file path references or replace them with the correct current paths under onchain/contracts/escrow_contract/test_snapshots/ (or simply delete that bullet list) so the comment no longer points to non-existent files.onchain/contracts/factory_contract/src/storage.rs (1)
1-189:⚠️ Potential issue | 🔴 CriticalCritical: unresolved merge conflict markers — file will not compile.
Conflicts at lines 1–4, 9–150, 157–188 must be resolved. The architectural choice between HEAD and upstream/dev carries implications beyond role deletion (HEAD drops
set_owner/get_owner/set_admin/get_admin/set_operator/get_operator).Two critical design issues to understand before choosing:
HEAD path delegates TTL management to shared module: HEAD imports
shared::storagefunctions which internally callbump_persistent()using the shared module's hardcoded constants (PERSISTENT_BUMP_AMOUNT = 518_400, PERSISTENT_LIFETIME_THRESHOLD = 120_960). The per-contract constants defined here become dead code—they are never read. The sharedset_persistent()andget_persistent_with_ttl()functions do not accept TTL parameters, so per-contract customization is impossible.upstream/dev path retains per-contract control: Direct
env.storage()calls allow each contract to use its own TTL constants. While all contracts currently define identical values, this design preserves flexibility for future per-contract TTL customization.Verify before merging:
- Is the loss of per-contract TTL customization intentional, or should the shared abstraction be extended to accept TTL parameters?
- If all contracts must always share identical TTL values, the HEAD path is acceptable but the per-contract constant definitions should be removed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/factory_contract/src/storage.rs` around lines 1 - 189, Resolve the merge conflict by keeping the upstream/dev implementation (remove the <<<<<<<, =======, >>>>>>> markers and the HEAD variant that uses shared::storage) so the file uses env.storage() calls and preserves per-contract TTL control; ensure the per-contract constants PERSISTENT_BUMP_AMOUNT and PERSISTENT_LIFETIME_THRESHOLD remain, restore functions set_owner/get_owner, set_admin/get_admin, set_operator/get_operator, and the upstream-style implementations of set_auction_contract/get_auction_contract, set_core_contract/get_core_contract, set_username/get_username (including persistent().extend_ttl calls), and set_config/get_config; after cleaning the conflict markers, run a build to verify there are no remaining references to shared::storage and that types like DataKey, UsernameRecord, and DeployConfig resolve correctly.onchain/contracts/escrow_contract/src/lib.rs (1)
1-858:⚠️ Potential issue | 🔴 CriticalUnresolved merge conflict markers throughout — file will not compile.
This file is saturated with
<<<<<<< HEAD/=======/>>>>>>> upstream/devmarkers across nearly every entrypoint (initialize,create_vault,deposit,withdraw,schedule_payment,execute_scheduled,cancel_vault,setup_auto_pay,cancel_auto_pay,trigger_auto_pay, the read‑only getters, and the trailing helpers). Until each conflict is collapsed to a single side and every marker line is removed,cargo build,cargo test, andcargo clippy(the issue's stated acceptance criteria) will all fail. Given the PR's intent, theHEAD(shared_auth/*_or_panic) variant is the one to keep.Also worth a quick sanity pass after resolving:
delete_auto_payis imported fromcrate::storageand used incancel_auto_pay— make sure it still exists instorage.rsafter that file's conflicts are resolved.- The
shared::authimport (line 26) and theResult<(), EscrowError>return type oncancel_vault(line 448 vs. 458) come from the HEAD side and need to stay together.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/escrow_contract/src/lib.rs` around lines 1 - 858, The file contains unresolved git merge conflict markers; remove every `<<<<<<<`, `=======`, and `>>>>>>>` block and collapse each conflicted region by choosing the HEAD variant (which uses shared::auth, shared_auth::unwrap_or_panic/_or_panic helpers, require_vault_owner, and the Result-returning cancel_vault signature). Ensure the import of shared::auth remains, keep helper functions read_vault_config_or_panic, read_vault_state_or_panic, require_vault_owner, and resolve that use shared_auth, and verify delete_auto_pay is still exported from storage.rs so cancel_auto_pay compiles; after cleanup run cargo build/test/clippy to catch any remaining minor fixes.onchain/contracts/escrow_contract/src/storage.rs (1)
3-248:⚠️ Potential issue | 🔴 CriticalUnresolved merge conflict markers — file will not compile.
This file contains seven unresolved
<<<<<<< HEAD/=======/>>>>>>> upstream/devblocks (lines 3‑40, 48‑83, 90‑120, 126‑132, 137‑187, 193‑199, 204‑248).cargo build/cargo test/cargo clippycannot succeed in this state, which directly violates the linked issue's acceptance criteria. Resolve every conflict by selecting the intended side (theHEADshared‑storage variant per this PR's stated goal) and remove every conflict marker before merging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/escrow_contract/src/storage.rs` around lines 3 - 248, Resolve the merge conflict markers by choosing the HEAD/shared_storage implementation throughout: restore the top-level import "use shared::storage as shared_storage;", remove all <<<<<<</=======/>>>>>>> lines, and keep the HEAD versions of the functions read_vault_config, write_vault_config, read_vault_state, write_vault_state, increment_payment_id, read_registration_contract, write_registration_contract, write_scheduled_payment, increment_auto_pay_id, read_auto_pay_count, write_auto_pay, and read_auto_pay (i.e., calls to shared_storage::get_persistent/set_persistent and shared_storage::get_instance/set_instance) so the file compiles with the intended shared_storage API.onchain/contracts/core_contract/src/lib.rs (2)
3-273:⚠️ Potential issue | 🔴 CriticalUnresolved merge conflict markers — file will not compile.
Three conflict blocks remain (lines 3‑92, 115‑119, 127‑273). The contract
implblock itself is split by a marker, so the crate cannot build. Pick the intended side (the HEAD doc‑commented surface, per this PR's intent), delete every<<<<<<</=======/>>>>>>>line, then re‑runcargo build/cargo clippy/cargo testto satisfy the issue's acceptance criteria.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/core_contract/src/lib.rs` around lines 3 - 273, There are leftover merge conflict markers; remove all <<<<<<< / ======= / >>>>>>> lines and keep the HEAD version (the doc‑commented top and the original Contract impl surface) so the crate compiles; specifically, restore the types import to types::{ChainType, PrivacyMode, Proof, PublicSignals} (remove PermissionSet), remove the upstream/dev admin/operator methods and delegate methods (get_admin, set_admin, get_operator, set_operator, grant_delegate, revoke_delegate) that were introduced in the other branch, ensure the Contract impl contains the HEAD methods like initialize, get_smt_root, register_resolver, set_privacy_mode, resolve, transfer, add_shielded_address, etc., then run cargo build / cargo clippy / cargo test to verify the merge is clean and compiles.
127-273:⚠️ Potential issue | 🔴 CriticalMerge conflicts remain unresolved in lib.rs; resolving in favor of HEAD breaks contract ABI.
The file contains unresolved merge conflict markers (lines 3, 91–92, 115–117, 127–205–273). If HEAD is selected as-is, the following public entrypoints and type imports are dropped:
- Missing entrypoints:
get_admin,get_operator,set_admin,set_operator(exist in Admin impl, downstream section),grant_delegate,revoke_delegate(in lib.rs downstream section).- Missing type:
PermissionSet(defined in types.rs, used only downstream).- Broken signatures:
set_memoloses thecaller: Addressparameter;set_privacy_modeloses thecaller: Addressparameter.These are breaking ABI changes for any deployed contracts/SDKs. The underlying implementations exist in the modules; only the public contract interface wrappers in lib.rs are being dropped. Per Issue
#391("Preserve external contract interfaces unless other issues require changes"), restore the downstream versions of these entrypoints, signatures, and type import, or document the API changes separately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/core_contract/src/lib.rs` around lines 127 - 273, Remove the unresolved merge markers and restore the downstream public ABI entries and signatures so the contract interface matches existing implementations: reinstate get_admin, get_operator, set_admin, set_operator, grant_delegate, and revoke_delegate public functions (wrapping Admin:: and Registration:: methods), restore set_memo and set_privacy_mode signatures to accept the caller Address parameter (e.g., set_memo(e: Env, c: Address, cm: BytesN<32>, m: u64) and set_privacy_mode(e: Env, c: Address, h: BytesN<32>, m: PrivacyMode)), and ensure the PermissionSet type (from types.rs) is available for grant_delegate(referencing Registration::grant_delegate) and revoke_delegate; remove the conflict markers (<<<<<<<, =======, >>>>>>>) and keep the downstream versions of these functions to preserve the ABI.zk/tests/README_username_leaf.md (1)
1-138:⚠️ Potential issue | 🟠 MajorPR scope mismatch: documentation does not align with PR description.
The PR description (issue
#391, "extract shared authorization and storage helpers across onchain contracts") targets Rust Soroban contract code underonchain/contracts/...and a newonchain/sharedcrate. None of the files under review here (this README, zk circuits, sdk tests, sdk wallet detection) belong to that scope. The AI summary additionally calls out "unresolved Git merge conflict markers" elsewhere in the PR. Strongly recommend splitting these unrelated additions (zk + sdk surface area) into a separate PR so the onchain refactor can be reviewed and merged on its own merits — and to make the merge-conflict resolution tractable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@zk/tests/README_username_leaf.md` around lines 1 - 138, The PR includes unrelated ZK and SDK artifacts (e.g., README_username_leaf.md, zk/, sdk/) while the intended scope is the onchain refactor (onchain/contracts/... and new onchain/shared crate); split these unrelated files into a separate PR by removing README_username_leaf.md and any zk/sdk additions from this branch (or move them to a new branch/PR) and re-submit only the onchain changes (functions/classes in onchain/contracts and the onchain/shared crate) for this PR; also scan the branch for unresolved Git merge conflict markers and resolve them before reopening the focused onchain PR.
♻️ Duplicate comments (2)
onchain/contracts/escrow_contract/Cargo.toml (1)
16-28:⚠️ Potential issue | 🔴 CriticalUnresolved merge conflict markers — manifest does not parse.
Same defect as the other contract manifests. Resolve to
shared = { workspace = true }.🔧 Suggested resolution
[dependencies] soroban-sdk = { workspace = true } -<<<<<<< HEAD -<<<<<<< HEAD:gateway-contract/contracts/escrow_contract/Cargo.toml -shared = { workspace = true } -======= -shared = { path = "../../shared" } ->>>>>>> upstream/main:onchain/contracts/escrow_contract/Cargo.toml -======= -<<<<<<<< HEAD:gateway-contract/contracts/escrow_contract/Cargo.toml -shared = { workspace = true } -======== -shared = { path = "../../shared" } ->>>>>>>> upstream/dev:onchain/contracts/escrow_contract/Cargo.toml ->>>>>>> upstream/dev +shared = { workspace = true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/escrow_contract/Cargo.toml` around lines 16 - 28, The Cargo.toml contains unresolved Git merge conflict markers (e.g., <<<<<<<, =======, >>>>>>>) around the shared dependency entry; remove all conflict marker lines and ensure the manifest uses the resolved entry shared = { workspace = true } (replace any occurrence of shared = { path = "../../shared" } with shared = { workspace = true }), leaving a clean Cargo.toml with no conflict markers.onchain/contracts/core_contract/Cargo.toml (1)
16-28:⚠️ Potential issue | 🔴 CriticalUnresolved merge conflict markers — manifest does not parse.
Same pattern as
factory_contract/Cargo.tomlandescrow_contract/Cargo.toml. The pipeline’scargo metadatafailure is rooted here. Resolve to the workspace dependency for consistency with[workspace.dependencies.shared]inonchain/Cargo.toml.🔧 Suggested resolution
[dependencies] soroban-sdk = { workspace = true } -<<<<<<< HEAD -<<<<<<< HEAD:gateway-contract/contracts/core_contract/Cargo.toml -shared = { workspace = true } -======= -shared = { path = "../../shared" } ->>>>>>> upstream/main:onchain/contracts/core_contract/Cargo.toml -======= -<<<<<<<< HEAD:gateway-contract/contracts/core_contract/Cargo.toml -shared = { workspace = true } -======== -shared = { path = "../../shared" } ->>>>>>>> upstream/dev:onchain/contracts/core_contract/Cargo.toml ->>>>>>> upstream/dev +shared = { workspace = true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/core_contract/Cargo.toml` around lines 16 - 28, This Cargo.toml contains unresolved merge conflict markers around the shared dependency; remove the conflict markers and set the dependency to the workspace form to match the workspace dependencies (replace the entire conflicting block with shared = { workspace = true }); target the core_contract/Cargo.toml's shared dependency line so it is consistent with onchain/Cargo.toml and the other contracts (factory_contract and escrow_contract).
| <<<<<<< HEAD | ||
| <<<<<<< HEAD:gateway-contract/Cargo.toml | ||
| [workspace.dependencies.shared] | ||
| path = "contracts/shared" | ||
| ======= | ||
| [workspace.lints.clippy] | ||
| missing_docs_in_private_items = "warn" | ||
| unwrap_used = "warn" | ||
| >>>>>>> upstream/main:onchain/Cargo.toml | ||
| ======= | ||
| <<<<<<<< HEAD:gateway-contract/Cargo.toml | ||
| [workspace.dependencies.shared] | ||
| path = "contracts/shared" | ||
| ======== | ||
| [workspace.lints.clippy] | ||
| missing_docs_in_private_items = "warn" | ||
| unwrap_used = "warn" | ||
| >>>>>>>> upstream/dev:onchain/Cargo.toml | ||
| >>>>>>> upstream/dev |
There was a problem hiding this comment.
Unresolved merge conflict markers — workspace manifest does not parse.
The two alternates are not actually competing: HEAD adds [workspace.dependencies.shared], dev re-adds [workspace.lints.clippy]. Both must be retained. Without this, every contract crate that resolves shared = { workspace = true } will also fail.
🔧 Suggested resolution
-<<<<<<< HEAD
-<<<<<<< HEAD:gateway-contract/Cargo.toml
-[workspace.dependencies.shared]
-path = "contracts/shared"
-=======
-[workspace.lints.clippy]
-missing_docs_in_private_items = "warn"
-unwrap_used = "warn"
->>>>>>> upstream/main:onchain/Cargo.toml
-=======
-<<<<<<<< HEAD:gateway-contract/Cargo.toml
-[workspace.dependencies.shared]
-path = "contracts/shared"
-========
-[workspace.lints.clippy]
-missing_docs_in_private_items = "warn"
-unwrap_used = "warn"
->>>>>>>> upstream/dev:onchain/Cargo.toml
->>>>>>> upstream/dev
+[workspace.lints.clippy]
+missing_docs_in_private_items = "warn"
+unwrap_used = "warn"
+
+[workspace.dependencies.shared]
+path = "contracts/shared"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <<<<<<< HEAD | |
| <<<<<<< HEAD:gateway-contract/Cargo.toml | |
| [workspace.dependencies.shared] | |
| path = "contracts/shared" | |
| ======= | |
| [workspace.lints.clippy] | |
| missing_docs_in_private_items = "warn" | |
| unwrap_used = "warn" | |
| >>>>>>> upstream/main:onchain/Cargo.toml | |
| ======= | |
| <<<<<<<< HEAD:gateway-contract/Cargo.toml | |
| [workspace.dependencies.shared] | |
| path = "contracts/shared" | |
| ======== | |
| [workspace.lints.clippy] | |
| missing_docs_in_private_items = "warn" | |
| unwrap_used = "warn" | |
| >>>>>>>> upstream/dev:onchain/Cargo.toml | |
| >>>>>>> upstream/dev | |
| [workspace.lints.clippy] | |
| missing_docs_in_private_items = "warn" | |
| unwrap_used = "warn" | |
| [workspace.dependencies.shared] | |
| path = "contracts/shared" |
🧰 Tools
🪛 GitHub Actions: Smart Contracts CI
[error] 13-13: cargo metadata failed: error parsing Cargo.toml. 'key with no value, expected ='.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@onchain/Cargo.toml` around lines 13 - 31, Remove the unresolved Git conflict
markers and restore both workspace entries so the manifest parses: delete the
<<<<<<</=======/>>>>>>> lines and keep both the [workspace.dependencies.shared]
table (path = "contracts/shared") and the [workspace.lints.clippy] table
(missing_docs_in_private_items = "warn", unwrap_used = "warn") in
onchain/Cargo.toml, ensuring valid TOML formatting and no duplicated headers so
crates that use shared = { workspace = true } can resolve correctly.
| <<<<<<< HEAD | ||
| // Re-export shared error codes | ||
| ======= | ||
| >>>>>>> upstream/dev | ||
| pub use shared::errors::AuctionError; |
There was a problem hiding this comment.
Critical: unresolved Git merge conflict markers.
Lines 1, 3, and 4 still contain conflict markers. Resolve by keeping the HEAD comment so the re-export remains valid.
Note: the pub use shared::errors::AuctionError; on line 5 also depends on shared/src/lib.rs declaring pub mod errors;, flagged separately at the root cause in the shared crate.
🛠️ Proposed resolution
-<<<<<<< HEAD
// Re-export shared error codes
-=======
->>>>>>> upstream/dev
pub use shared::errors::AuctionError;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <<<<<<< HEAD | |
| // Re-export shared error codes | |
| ======= | |
| >>>>>>> upstream/dev | |
| pub use shared::errors::AuctionError; | |
| // Re-export shared error codes | |
| pub use shared::errors::AuctionError; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@onchain/contracts/auction_contract/src/errors.rs` around lines 1 - 5, Remove
the unresolved Git conflict markers (<<<<<<<, =======, >>>>>>>) from errors.rs
and keep the intended HEAD comment line so the file contains only the re-export
line pub use shared::errors::AuctionError;; ensure the re-export compiles by
confirming the shared crate exposes the module (declare pub mod errors; in
shared::lib.rs) so shared::errors::AuctionError resolves.
| <<<<<<< HEAD | ||
| // Event symbols must be <= 9 chars (Soroban `symbol_short!`). | ||
| ======= | ||
| >>>>>>> upstream/dev |
There was a problem hiding this comment.
Unresolved merge conflict — comment-only.
Keep the HEAD-side comment (it's a useful invariant about the 9-char symbol_short! limit) and drop the markers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@onchain/contracts/auction_contract/src/events.rs` around lines 3 - 6, Remove
the unresolved merge markers and keep the HEAD comment line about the 9-char
limit: delete the lines "<<<<<<< HEAD", "=======", and ">>>>>>> upstream/dev"
and ensure the remaining comment reads "// Event symbols must be <= 9 chars
(Soroban `symbol_short!`)." so the invariant about symbol_short! is preserved
and no merge markers remain in the file.
| <<<<<<< HEAD | ||
| #[allow(deprecated)] | ||
| env.events().publish( | ||
| (BID_PLACED, username_hash.clone()), | ||
| (bidder.clone(), amount), | ||
| ); | ||
| ======= | ||
| BidPlacedEvent { | ||
| username_hash: username_hash.clone(), | ||
| bidder: bidder.clone(), | ||
| amount, | ||
| } | ||
| .publish(env); | ||
| >>>>>>> upstream/dev |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm tests rely on (BID_PLACED, hash) topic shape and (bidder, amount) data.
rg -nP --type=rust -C5 'BID_PLACED|BID_REFUNDED' onchain/contracts/auction_contract
rg -nP --type=rust -C3 'BidPlacedEvent|BidRefundedEvent'Repository: Alien-Protocol/Alien-Protocol
Length of output: 7690
🏁 Script executed:
# Find test files that might assert on event topics or structure
find . -type f -name "*.rs" -path "*/test*" -o -name "*test*.rs" | head -20Repository: Alien-Protocol/Alien-Protocol
Length of output: 474
🏁 Script executed:
# Search for test assertions on BID_PLACED or BID_REFUNDED events
rg -n --type=rust '(assert|expect|BID_PLACED|BID_REFUNDED)' onchain/contracts/auction_contract --type-list | grep -i testRepository: Alien-Protocol/Alien-Protocol
Length of output: 463
🏁 Script executed:
# Get full context around BidRefundedEvent struct to verify dead_code attribute
sed -n '48,58p' onchain/contracts/auction_contract/src/events.rsRepository: Alien-Protocol/Alien-Protocol
Length of output: 364
🏁 Script executed:
# Search for test assertions on events in the test file
rg -n --type=rust 'BID_PLACED|BID_REFUNDED|topic|payload|event' onchain/contracts/auction_contract/src/test.rs -C2 | head -100Repository: Alien-Protocol/Alien-Protocol
Length of output: 3499
🏁 Script executed:
# Look for any event decoding or assertions
grep -n 'assert\|expect\|\.0\|\.1' onchain/contracts/auction_contract/src/test.rs | head -50Repository: Alien-Protocol/Alien-Protocol
Length of output: 3081
Accept HEAD and add #[allow(dead_code)] to BidRefundedEvent.
The event implementations emit observably different event symbols and topic structures:
- HEAD:
env.events().publish((BID_PLACED, username_hash), (bidder, amount))→ emits"BID_PLCD"symbol in topic[0] - upstream/dev:
BidPlacedEvent { … }.publish(env)→ emits"bid_placed_event"in topic[0]
The test assertions (in test.rs) are also conflicted and expect different symbols. The HEAD test variant explicitly looks for "BID_PLCD" and "BID_RFDN" symbols in topics[0] and decodes data as (Address, i128). Choosing upstream/dev would misalign the emitted events with the HEAD test expectations.
Accepting HEAD leaves both BidPlacedEvent and BidRefundedEvent as unused types. BidPlacedEvent already has #[allow(dead_code)]; add the same annotation to BidRefundedEvent (line 51 in events.rs).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@onchain/contracts/auction_contract/src/events.rs` around lines 68 - 81,
Choose the HEAD implementation for emitting events (keep the
env.events().publish((BID_PLACED, username_hash.clone()), (bidder.clone(),
amount)) call instead of constructing BidPlacedEvent and calling .publish(env))
so emitted topic symbols remain unchanged for tests, and add #[allow(dead_code)]
above the BidRefundedEvent type (the type declared around line 51 as
BidRefundedEvent) to silence unused-type warnings; ensure BidPlacedEvent retains
its existing #[allow(dead_code)] attribute.
| } | ||
|
|
||
| // Main component for standalone compilation and testing | ||
| component main {public [leaf]} = UsernameLeaf(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Circom 2 component main public list inputs vs outputs
💡 Result:
In Circom 2, the main component defines the circuit's interface. Its syntax is component main {public [signal_list]} = Template; where {public [signal_list]} is optional. Inputs: All input signals to the main component's template are private by default. Only those explicitly listed in {public [signal_list]} become public inputs, visible to the verifier in the proof. Outputs: All output signals of the main component are always public. They cannot be made private and are included in the public signals of the witness. Key differences: - Public inputs (specified in the list): Provided by the prover, known to verifier, must satisfy constraints. - Private inputs (not in list): Hidden from verifier. - Outputs: Computed from inputs, always public, verifier checks they match constraints. No direct "return" mechanism; they are public signals in the witness. Example: template Multiplier { signal input a; signal input b; signal output c; c <== a * b; } component main {public [a]} = Multiplier; // a public input, b private input, c public output Witness structure: [1, public signals (a, c), private signals (b), intermediates].
Citations:
- 1: https://docs.circom.io/circom-language/the-main-component/
- 2: https://rareskills.io/post/circom-public-private-inputs
- 3: https://docs.circom.io/circom-language/signals/
🏁 Script executed:
# Find the UsernameLeaf component definition
find . -name "*.circom" -type f | head -20Repository: Alien-Protocol/Alien-Protocol
Length of output: 329
🏁 Script executed:
# Search for UsernameLeaf template definition
rg "template UsernameLeaf" -A 20Repository: Alien-Protocol/Alien-Protocol
Length of output: 1054
public [leaf] is incorrect — leaf is an output, not an input.
In Circom 2, the public [...] list of component main enumerates public inputs of the main component. Output signals are always public by definition. Listing the output leaf here is at minimum redundant and will fail with an error such as "the signal is not an input of the main component."
If username is meant to be the public input, declare that instead; otherwise drop the public [...] clause entirely so that only leaf (implicitly public as an output) is exposed.
🔧 Proposed fix
-component main {public [leaf]} = UsernameLeaf();
+component main = UsernameLeaf();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| component main {public [leaf]} = UsernameLeaf(); | |
| component main = UsernameLeaf(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@zk/circuits/merkle/username_leaf_main.circom` at line 18, The main component
declaration wrongly marks the output signal "leaf" as a public input; update the
component main declaration for UsernameLeaf by either removing the entire
"public [leaf]" clause so that leaf remains an implicit public output, or if the
intended public input is the username signal, change the list to "public
[username]" to expose the input instead; locate the declaration "component main
{public [leaf]} = UsernameLeaf();" and modify it to match the correct signal
("leaf" -> remove, or "username" -> expose) in the UsernameLeaf circuit.
| @@ -0,0 +1,5 @@ | |||
| pragma circom 2.0.0; | |||
|
|
|||
| include "username_hash.circom"; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find any username_hash*.circom files and verify the include target exists
fd -e circom 'username_hash' zk/circuits
# Also check for any UsernameHash template definitions to confirm the include provides it
rg -nP '\btemplate\s+UsernameHash\s*\(' --type-add 'circom:*.circom' --type=circomRepository: Alien-Protocol/Alien-Protocol
Length of output: 215
Fix the include path to reference the correct file.
The include statement references username_hash.circom, which does not exist in the repository. The UsernameHash template is defined in username_hash_impl.circom. Change the include to:
include "username_hash_impl.circom";
The current path will fail at compile time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@zk/circuits/username_hash_main.circom` at line 3, The include line currently
imports a non-existent file; update the include in username_hash_main.circom to
reference the correct implementation file by replacing the import of
"username_hash.circom" with "username_hash_impl.circom" so the UsernameHash
template defined in username_hash_impl.circom is available at compile time.
| async function main() { | ||
| // Test 1: valid ASCII username should generate a witness | ||
| { | ||
| const input = { username: encodeUsername("alice") }; | ||
| const { wtns } = await snarkjs.wtns.calculate(input, WASM_PATH, {}); | ||
| assert.ok(wtns, "Witness should be generated for valid ASCII input"); | ||
| } | ||
|
|
||
| // Test 2: all-zero (null bytes) input should be accepted (0 <= 127) | ||
| { | ||
| const input = { username: new Array(32).fill(0) }; | ||
| await snarkjs.wtns.calculate(input, WASM_PATH, {}); | ||
| } | ||
|
|
||
| // Test 3: value 127 (DEL) is the boundary — must be accepted | ||
| { | ||
| const input = { username: new Array(32).fill(127) }; | ||
| await snarkjs.wtns.calculate(input, WASM_PATH, {}); | ||
| } | ||
|
|
||
| // Test 4: value 128 must be rejected by the circuit | ||
| { | ||
| const input = { username: new Array(32).fill(0) }; | ||
| input.username[0] = 128; | ||
| let rejected = false; | ||
| try { | ||
| await snarkjs.wtns.calculate(input, WASM_PATH, {}); | ||
| } catch (_) { | ||
| rejected = true; | ||
| } | ||
| assert.ok(rejected, "Value 128 should be rejected by range constraint"); | ||
| } | ||
|
|
||
| // Test 5: large out-of-range value (255) must be rejected | ||
| { | ||
| const input = { username: new Array(32).fill(0) }; | ||
| input.username[15] = 255; | ||
| let rejected = false; | ||
| try { | ||
| await snarkjs.wtns.calculate(input, WASM_PATH, {}); | ||
| } catch (_) { | ||
| rejected = true; | ||
| } | ||
| assert.ok(rejected, "Value 255 should be rejected by range constraint"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What does snarkjs.wtns.calculate(input, wasmFile, wtnsFileName) return programmatically, and is the third argument required?
💡 Result:
snarkjs.wtns.calculate(input, wasmFile, wtnsFileName) is an asynchronous function that calculates the witness for a given input using the provided WASM circuit file and writes the binary witness data to the specified wtnsFileName file on disk. It returns a Promise that resolves when the calculation is complete and the file has been successfully written (typically resolving to undefined, as is common for async file-write operations in such libraries). The third argument, wtnsFileName, is required, as the function is designed primarily to generate and save the witness file to disk; it does not return the witness data directly in memory. For obtaining the witness as a buffer (e.g., in browser environments without file system access), use the circom-generated JS code or circom_runtime.WitnessCalculatorBuilder instead, as noted in snarkjs issues.
Citations:
- 1: Generating just a witness in browser iden3/snarkjs#459
- 2: https://www.npmjs.com/package/snarkjs?activeTab=readme
- 3: https://www.npmjs.com/package/snarkjs
- 4: https://github.com/iden3/snarkjs/tree/master
🏁 Script executed:
fd -t f "test_username_range_check.js" | head -5Repository: Alien-Protocol/Alien-Protocol
Length of output: 111
🏁 Script executed:
cat -n zk/tests/test_username_range_check.js | sed -n '1,90p'Repository: Alien-Protocol/Alien-Protocol
Length of output: 3092
Fix snarkjs.wtns.calculate() calls: return value is undefined and third argument must be a filename string, not an object.
snarkjs.wtns.calculate(input, wasmFile, wtnsFileName) writes the witness to disk and returns undefined. Line 42 destructures { wtns } from the return value (which is undefined), causing the assertion on line 43 to fail. All calls (lines 42, 49, 55, 64, 77) pass {} as the third argument, but a string filename is required.
Add fs and os imports, then pass a temp file path for each test:
Suggested fix
const path = require("path");
const assert = require("assert");
+const fs = require("fs");
+const os = require("os");
const snarkjs = require("snarkjs");
const CIRCUIT = "username_hash";
const BUILD_DIR = path.join(__dirname, "..", "build", CIRCUIT);
const WASM_PATH = path.join(
BUILD_DIR,
"wasm",
`${CIRCUIT}_js`,
`${CIRCUIT}.wasm`
);
+const tmpWtns = () => path.join(os.tmpdir(), `range-${process.pid}-${Date.now()}.wtns`);
async function main() {
// Test 1: valid ASCII username should generate a witness
{
const input = { username: encodeUsername("alice") };
- const { wtns } = await snarkjs.wtns.calculate(input, WASM_PATH, {});
- assert.ok(wtns, "Witness should be generated for valid ASCII input");
+ const out = tmpWtns();
+ await snarkjs.wtns.calculate(input, WASM_PATH, out);
+ assert.ok(fs.existsSync(out), "Witness should be generated for valid ASCII input");
+ fs.unlinkSync(out);
}
// Test 2: all-zero (null bytes) input should be accepted (0 <= 127)
{
const input = { username: new Array(32).fill(0) };
- await snarkjs.wtns.calculate(input, WASM_PATH, {});
+ const out = tmpWtns();
+ await snarkjs.wtns.calculate(input, WASM_PATH, out);
+ fs.unlinkSync(out);
}
// Test 3: value 127 (DEL) is the boundary — must be accepted
{
const input = { username: new Array(32).fill(127) };
- await snarkjs.wtns.calculate(input, WASM_PATH, {});
+ const out = tmpWtns();
+ await snarkjs.wtns.calculate(input, WASM_PATH, out);
+ fs.unlinkSync(out);
}
// Test 4: value 128 must be rejected by the circuit
{
const input = { username: new Array(32).fill(0) };
input.username[0] = 128;
let rejected = false;
try {
- await snarkjs.wtns.calculate(input, WASM_PATH, {});
+ const out = tmpWtns();
+ await snarkjs.wtns.calculate(input, WASM_PATH, out);
+ fs.unlinkSync(out);
} catch (_) {
rejected = true;
}
assert.ok(rejected, "Value 128 should be rejected by range constraint");
}
// Test 5: large out-of-range value (255) must be rejected
{
const input = { username: new Array(32).fill(0) };
input.username[15] = 255;
let rejected = false;
try {
- await snarkjs.wtns.calculate(input, WASM_PATH, {});
+ const out = tmpWtns();
+ await snarkjs.wtns.calculate(input, WASM_PATH, out);
+ fs.unlinkSync(out);
} catch (_) {
rejected = true;
}
assert.ok(rejected, "Value 255 should be rejected by range constraint");
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@zk/tests/test_username_range_check.js` around lines 38 - 82,
snarkjs.wtns.calculate() returns undefined and expects the third parameter to be
a filepath string, so remove the destructuring of "{ wtns }" and supply a
temporary filename for every call to snarkjs.wtns.calculate(input, WASM_PATH,
wtnsFileName); add imports for fs and os, generate a temp path (e.g., use
os.tmpdir() + unique name) per test, pass that string as the third arg, and for
success cases assert that the temp file exists (or simply that the call did not
throw); keep the same inputs (encodeUsername, WASM_PATH, the username arrays)
and keep the try/catch-based assertions for rejection tests but still pass a
temp filename there too.
| import * as path from "path"; | ||
| import * as assert from "assert"; | ||
| import * as fs from "fs"; | ||
| import { snarkjs } from "snarkjs"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm how snarkjs is imported elsewhere in this repo.
rg -nP --type=ts --type=js '\bfrom\s+"snarkjs"|require\(\s*"snarkjs"\s*\)'Repository: Alien-Protocol/Alien-Protocol
Length of output: 610
Wrong snarkjs import shape — module exports a default, not a named export.
The file uses import { snarkjs } from "snarkjs", which resolves to undefined. Every other file in the codebase imports it correctly as a default export using import snarkjs from "snarkjs" (or const snarkjs = require("snarkjs") in CommonJS). The named import will cause TypeError: Cannot read properties of undefined at runtime when snarkjs.* methods are called.
Fix
-import { snarkjs } from "snarkjs";
+import snarkjs from "snarkjs";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { snarkjs } from "snarkjs"; | |
| import snarkjs from "snarkjs"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@zk/tests/username_leaf_test.ts` at line 4, The import of snarkjs uses a named
import which yields undefined; replace the named import "import { snarkjs } from
'snarkjs'" with the default-style import (or CommonJS require) so that the
module's default export is used; update the import line that references snarkjs
in username_leaf_test.ts (the snarkjs symbol) to use the default import form so
subsequent snarkjs.* calls work at runtime.
| // Generate witness using circuit | ||
| console.log("\n── Generating circuit witness ───────────────────────────────"); | ||
|
|
||
| const wasmBuffer = fs.readFileSync(WASM_PATH); | ||
| const { witness } = await snarkjs.wtns.calculateWitnessFromBuffer(wasmBuffer, input); | ||
| console.log("✓ Witness generated successfully"); | ||
|
|
||
| // Extract leaf output from witness | ||
| // The leaf output should be at index 1 (after the first signal which is usually a dummy) | ||
| const circuitHash = BigInt(witness[1]); | ||
| console.log(`✓ Circuit hash output: ${circuitHash.toString()}`); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look in installed snarkjs (or repo) for the symbol used here.
fd -t f 'package.json' -d 4 | head -n 5
rg -nP 'calculateWitnessFromBuffer|wtns\.calculate\b' --type=js --type=ts -C2Repository: Alien-Protocol/Alien-Protocol
Length of output: 2669
🌐 Web query:
Does snarkjs export wtns.calculateWitnessFromBuffer? What is the documented programmatic API for in-memory witness calculation?
💡 Result:
No, snarkjs does not export wtns.calculateWitnessFromBuffer. No sources mention this specific function. The documented programmatic API for in-memory witness calculation in snarkjs is snarkjs.wtns.calculate(input, wasmBuffer, witnessBuffer, options?), which computes the witness directly into a provided Uint8Array buffer without file I/O. Example: await snarkjs.wtns.calculate(input, wasmBuffer, witnessBuffer, {memorySize: 0}); This is shown in the snarkjs README and npm page for controlling memory allocation in web environments. snarkjs.wtns.calculate is primarily CLI-oriented but supports programmatic buffer usage. For pure in-memory calculation without snarkjs, use circom_runtime's WitnessCalculatorBuilder(wasmBuffer), which provides calculateWTNSBin(input) or calculateBinWitness(input) returning Uint8Array witness buffers. This is the recommended browser/programmatic approach per GitHub issues, as circom generates compatible JS/WASM code.
Citations:
- 1: Generating just a witness in browser iden3/snarkjs#459
- 2: https://github.com/iden3/snarkjs
- 3: https://www.npmjs.com/package/snarkjs
- 4: https://github.com/iden3/snarkjs/tree/master
- 5: Error while running groth16.fullProve in Node (wc.calculateBinWitness is not a function) iden3/snarkjs#126
🏁 Script executed:
cd zk && cat package.json | grep -A 5 -B 5 snarkjsRepository: Alien-Protocol/Alien-Protocol
Length of output: 118
🏁 Script executed:
# Check if there's a node_modules or lock file that shows actual version
cd zk && (cat package-lock.json 2>/dev/null || cat yarn.lock 2>/dev/null) | grep -A 3 '"snarkjs"' | head -20Repository: Alien-Protocol/Alien-Protocol
Length of output: 55
🏁 Script executed:
fd -n 'package.json' | grep zk | head -5Repository: Alien-Protocol/Alien-Protocol
Length of output: 304
🏁 Script executed:
# Find zk directory structure
fd -t d 'zk' -d 2Repository: Alien-Protocol/Alien-Protocol
Length of output: 77
🏁 Script executed:
# Read the zk package.json if it exists
find . -path "*/zk/package.json" -exec cat {} \;Repository: Alien-Protocol/Alien-Protocol
Length of output: 55
🏁 Script executed:
cat zk/package.jsonRepository: Alien-Protocol/Alien-Protocol
Length of output: 121
🏁 Script executed:
# Also check the test files to understand the patterns better
head -20 zk/tests/username_leaf_test.tsRepository: Alien-Protocol/Alien-Protocol
Length of output: 807
🏁 Script executed:
# Find all package.json files in the repo
find . -name "package.json" -type fRepository: Alien-Protocol/Alien-Protocol
Length of output: 130
🏁 Script executed:
# Check root and backend package.json for snarkjs
cat package.json | grep -A 2 snarkjsRepository: Alien-Protocol/Alien-Protocol
Length of output: 55
🏁 Script executed:
# Also check backend
cat backend/package.json | grep -A 2 snarkjsRepository: Alien-Protocol/Alien-Protocol
Length of output: 55
🏁 Script executed:
cat ./package.jsonRepository: Alien-Protocol/Alien-Protocol
Length of output: 255
🏁 Script executed:
cat ./backend/package.jsonRepository: Alien-Protocol/Alien-Protocol
Length of output: 2539
🏁 Script executed:
# Check for snarkjs in all package.json files
grep -r "snarkjs" . --include="package.json"Repository: Alien-Protocol/Alien-Protocol
Length of output: 116
🏁 Script executed:
# Check sdk/package.json for full snarkjs context
cat ./sdk/package.jsonRepository: Alien-Protocol/Alien-Protocol
Length of output: 1047
🏁 Script executed:
# Look at the imports in the test file more carefully
head -50 zk/tests/username_leaf_test.tsRepository: Alien-Protocol/Alien-Protocol
Length of output: 1747
🏁 Script executed:
# Check if there's a tsconfig or build config in zk directory
find zk -maxdepth 2 -name "tsconfig.json" -o -name "package.json" -o -name "*.config.*"Repository: Alien-Protocol/Alien-Protocol
Length of output: 90
🏁 Script executed:
# Check the test_username_range_check.js file to see how it uses snarkjs
cat zk/tests/test_username_range_check.js | head -80Repository: Alien-Protocol/Alien-Protocol
Length of output: 2286
🏁 Script executed:
# Search for snarkjs version 0.7.0 API info
rg "wtns\." zk/tests/ -A 2 -B 2Repository: Alien-Protocol/Alien-Protocol
Length of output: 2954
🏁 Script executed:
# Check snarkjs documentation or types in node_modules if available
find . -path "*/node_modules/snarkjs*" -name "*.d.ts" 2>/dev/null | head -5Repository: Alien-Protocol/Alien-Protocol
Length of output: 55
🏁 Script executed:
# Get the full snarkjs 0.7.0 documentation or source
web_search "snarkjs 0.7.0 wtns calculate api"Repository: Alien-Protocol/Alien-Protocol
Length of output: 124
🏁 Script executed:
# Check the snarkjs README in the repo if any mentions of witness calculation exist
find . -name "README*" -type f -exec grep -l "witness\|wtns" {} \;Repository: Alien-Protocol/Alien-Protocol
Length of output: 108
🏁 Script executed:
# Look at what was attempted in the README snippet
cat zk/tests/README_username_leaf.md | grep -A 5 -B 5 "calculateWitness"Repository: Alien-Protocol/Alien-Protocol
Length of output: 444
🏁 Script executed:
# Let me check snarkjs 0.7.0 docs online via web_search capabilityRepository: Alien-Protocol/Alien-Protocol
Length of output: 55
🌐 Web query:
snarkjs 0.7.0 wtns.calculate API documentation signature parameters
💡 Result:
In snarkjs (including the 0.7.x line), the “wtns.calculate” witness-calculation API takes the following arguments: 1) input: the circuit input object 2) wasmFile: the compiled circuit .wasm (path/identifier or bytes, depending on the wrapper) 3) wtns: the output witness file (for the file-based API) 4) options: an optional options object (only documented key found: memorySize) Example (from the official snarkjs npm docs): await wtnsCalculate(input, wasmFile, wtns, { memorySize: 0 }); The only explicitly documented parameter inside the options object in the sources located here is memorySize (used to control initial memory allocation in the witness calculator). Also note: snarkjs’ docs/CLI show that witness calculation is commonly done via the generated circom witness calculator (or equivalent command), e.g.: snarkjs wtns calculate circuit_js/circuit.wasm input.json witness.wtns If you need the exact TypeScript/JS signature and all supported options for snarkjs 0.7.0 specifically, the public sources above don’t include that full signature text; however, later work in the repository (2024) indicates witness-calc options support (notably memorySize).
Citations:
- 1: https://github.com/iden3/snarkjs
- 2: https://www.npmjs.com/package/snarkjs
- 3: https://registry.npmjs.org/snarkjs
- 4: https://github.com/iden3/snarkjs/tree/master
- 5: Support for wtns calc and prover options iden3/snarkjs#517
🏁 Script executed:
# Check if there's any custom snarkjs wrapper or patch in the repo
find . -path ./node_modules -prune -o -name "*snarkjs*" -type f -printRepository: Alien-Protocol/Alien-Protocol
Length of output: 55
🏁 Script executed:
# Look for any type definitions or imports that might clarify the API
rg "snarkjs" zk/ --type=ts -B 2 -A 2 | head -60Repository: Alien-Protocol/Alien-Protocol
Length of output: 2425
🏁 Script executed:
# Check the exact snarkjs 0.7.0 from npm (simulate what would be installed)
python3 << 'EOF'
import json
import subprocess
# Try to find what the 0.7.0 API actually exports
print("Checking snarkjs 0.7.0 API...")
print("\nBased on npm registry and GitHub sources:")
print("- snarkjs 0.7.0 does NOT export 'calculateWitnessFromBuffer'")
print("- The API in 0.7.x is: snarkjs.wtns.calculate(input, wasmFile, wtns, options)")
print("- Return value structure: { wtns } or success-based (not { witness })")
print("\nAttempted usage in code:")
print(" const { witness } = await snarkjs.wtns.calculateWitnessFromBuffer(wasmBuffer, input)")
print("\nThis WILL fail at runtime because the function does not exist.")
EOFRepository: Alien-Protocol/Alien-Protocol
Length of output: 532
🌐 Web query:
snarkjs 0.7.0 calculateWitnessFromBuffer deprecated old API changelog
💡 Result:
I couldn’t find any authoritative source (official snarkjs repo docs, release notes, or changelog entries) that mentions “calculateWitnessFromBuffer” or says it was deprecated in snarkjs v0.7.0. What I did find (relevant to witness calculation APIs around the same era/version line): 1) snarkjs v0.7.0: no accessible changelog/release-note text confirming that specific function rename/deprecation. (The sources I found around v0.7.x are general release pages; the search results surfaced v0.7.5 and v0.7.6, not a v0.7.0 release note mentioning the Buffer witness API.) 2) For witness generation “as bytes/buffers”, snarkjs users commonly rely on circom_runtime’s WitnessCalculatorBuilder and then call either calculateBinWitness (circom v1) or calculateWTNSBin (circom v2+), avoiding the older “write witness.wtns to disk” style. A closed issue explicitly shows this approach to “Calc witness as bytes without saving any files”. 3) Another snarkjs issue about fullProve failing shows that the witness calculator’s available methods depend on the witness calculator implementation; the suggested workaround uses witnessCalculator.calculateWTNSBin(...) and then snarkjs.groth16.prove(..., witnessBuffer). Practical conclusion for your query: - Treat “calculateWitnessFromBuffer” as an undocumented/unsupported API name in the sources I located. Instead, for buffer-based witness calculation compatible with current snarkjs usage patterns, use circom_runtime’s WitnessCalculatorBuilder and the appropriate calculateWTNSBin/calculateBinWitness method (depending on circom major version). If you share the exact code snippet you’re using (and where you saw calculateWitnessFromBuffer), I can map it to the corresponding modern snarkjs/circom_runtime call pattern and also determine whether it existed in any snarkjs v0.7.0-specific build.
Citations:
- 1: Error while running groth16.fullProve in Node (wc.calculateBinWitness is not a function) iden3/snarkjs#126
- 2: Generating just a witness in browser iden3/snarkjs#459
- 3: https://github.com/iden3/snarkjs/releases
- 4: https://github.com/iden3/snarkjs
- 5: https://github.com/iden3/snarkjs/releases/tag/v0.7.5
- 6: iden3/snarkjs@9a8f1c0
Replace snarkjs.wtns.calculateWitnessFromBuffer with the documented API.
snarkjs.wtns.calculateWitnessFromBuffer does not exist in snarkjs 0.7.0 and will fail at runtime with a "is not a function" error. The documented programmatic API is snarkjs.wtns.calculate(input, wasmPath, wtnsFileName), which writes to a file. For in-memory witness calculation, use circom_runtime's WitnessCalculatorBuilder(wasmBuffer) followed by wc.calculateWTNSBin(input) or wc.calculateBinWitness(input) (depending on your circom version), which returns a Uint8Array.
Alternatively, if you must use snarkjs, call snarkjs.wtns.calculate(input, WASM_PATH, tmpFile) and then parse the witness file.
This issue occurs at lines 104 and 133.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@zk/tests/username_leaf_test.ts` around lines 100 - 110, The test currently
calls the non-existent snarkjs.wtns.calculateWitnessFromBuffer; replace this
with a supported approach: either use circom_runtime's
WitnessCalculatorBuilder(wasmBuffer) to build a witness calculator and call
wc.calculateWTNSBin(input) or wc.calculateBinWitness(input) to get a Uint8Array
witness in-memory (then parse it), or call the documented snarkjs API
snarkjs.wtns.calculate(input, WASM_PATH, tmpFile) to write a wtns file and
read/parse it; update the code paths that reference
snarkjs.wtns.calculateWitnessFromBuffer and any subsequent witness handling
(e.g., reading witness[1]) to use the chosen method and its output format.
ryzen-xp
added
REJECTED
CI Failed
bamiebot-maker
force-pushed
the
refactor/extract-shared-onchain-helpers
branch
from
e69348a to
98783aa
Compare
|
all check passed and no merger conflict |
ryzen-xp
added
APPROVED
2 participants
Fixes #391
Extracted shared auth layers and storage wrappers into a unified reusable
sharedcrate in the workspace, drastically reducing the boilerplate pattern mapping across core,escrow,factory, and auction contracts.Option<T>lookup paths over persistent unwrap checks.Summary by CodeRabbit
Refactor
Chores