Skip to content

Refactor/fix repeated parameters#76

Merged
armandocodecr merged 3 commits intodevelopfrom
refactor/fix-repeated-parameters
Mar 13, 2026
Merged

Refactor/fix repeated parameters#76
armandocodecr merged 3 commits intodevelopfrom
refactor/fix-repeated-parameters

Conversation

@armandocodecr
Copy link
Copy Markdown
Contributor

@armandocodecr armandocodecr commented Mar 13, 2026

Summary by CodeRabbit

  • New Features

    • Added admin authentication requirement for contract deployments to ensure authorized access
  • Refactor

    • Switched escrow identifiers from string IDs to contract address values across deployment and participation-token flows
    • Updated deployment flow to pass escrow contract addresses directly into token initialization
  • Tests

    • Updated tests to use address-based escrow values and adjusted helpers accordingly

@vercel
Copy link
Copy Markdown

vercel bot commented Mar 13, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
backoffice-tokenization Ready Ready Preview, Comment Mar 13, 2026 8:58am
investor Ready Ready Preview, Comment Mar 13, 2026 8:58am

Request Review

@armandocodecr armandocodecr requested a review from JoelVR17 March 13, 2026 08:33
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 13, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 760365c4-ec0d-4083-b6f6-c25008d51948

📥 Commits

Reviewing files that changed from the base of the PR and between 06a9f6c and 09c2d8b.

📒 Files selected for processing (2)
  • apps/smart-contracts/contracts/deployer/src/deployer.rs
  • apps/smart-contracts/contracts/deployer/src/test.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/smart-contracts/contracts/deployer/src/deployer.rs

📝 Walkthrough

Walkthrough

Refactors escrow representation across multiple contracts and tests: replaces String-based escrow_id with Address-based escrow_contract, updates constructor/getter/storage signatures and usages, and adjusts deployer/test call sites to pass Address values instead of string IDs.

Changes

Cohort / File(s) Summary
Deployer
apps/smart-contracts/contracts/deployer/src/deployer.rs, apps/smart-contracts/contracts/deployer/src/test.rs
Removed DeployAllParams.escrow_id: String; updated deploy_participation_token parameter to escrow_contract: Address; adjusted call sites to pass escrow_contract (and .clone() where needed) in deploy_all and tests.
Participation Token — Contract, Metadata, Storage, Tests
apps/smart-contracts/contracts/participation-token/src/contract.rs, .../metadata.rs, .../storage_types.rs, .../test.rs
Replaced escrow_id: String with escrow_contract: Address across constructor, public getter, metadata read/write functions; renamed DataKey::EscrowIdDataKey::EscrowContract; updated error messages and all test helpers/call sites to use Address.
Token-Sale & Vault Tests
apps/smart-contracts/contracts/token-sale/src/test.rs, apps/smart-contracts/contracts/vault-contract/src/test.rs
Updated test helpers (create_token_factory) to accept escrow_contract: &Address and pass generated/ referenced escrow Address into factory/token initialization instead of hard-coded/empty string escrow IDs.
Manifest
Cargo.toml
Small manifest changes (lines changed +47/-37) accompanying the code edits.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

Suggested reviewers

  • JoelVR17
  • zkCaleb-dev

Poem

🐰 I swapped the string for something stout,
A hopping Address, strong throughout,
Constructors, tests, and keys aligned,
No loose IDs left behind,
A tiny rabbit cheers the route! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'Refactor/fix repeated parameters' is vague and does not clearly convey the main change, which is replacing escrow_id (String) with escrow_contract (Address) across multiple contracts and files. Consider a more specific title like 'Replace escrow_id with escrow_contract Address' or 'Update escrow parameter from String ID to Address' to better describe the core refactoring.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch refactor/fix-repeated-parameters
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
apps/smart-contracts/contracts/deployer/src/deployer.rs (1)

20-35: ⚠️ Potential issue | 🟠 Major

Address-based escrow API change is not fully propagated to callers.

The contract struct now expects escrow_contract: Address only, but the codebase still sends stale escrow_id fields and encodes escrow contract as string instead of address, causing deployment transaction failures.

Required caller updates
# apps/core/src/deploy/deploy.service.ts (deployParticipationToken)
  escrow_id: dto.escrowContractId,
+ escrow_contract: dto.escrowContractId,
# apps/core/src/deploy/deploy.service.ts (deploy_all)
- escrow_id: dto.escrowId,
  escrow_contract: dto.escrowContract,
# apps/backoffice-tokenization/src/lib/tokenDeploymentService.ts
- client.nativeString(escrowContractId),
+ client.nativeAddress(escrowContractId),
# apps/investor-tokenization/src/lib/tokenDeploymentService.ts
- client.nativeString(escrowContractId),
+ client.nativeAddress(escrowContractId),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/smart-contracts/contracts/deployer/src/deployer.rs` around lines 20 -
35, DeployAllParams now expects escrow_contract: Address but callers still pass
a stale escrow_id and encode the escrow as a string; update every call site that
constructs DeployAllParams (and any helper builders) to stop using escrow_id and
instead pass the escrow_contract as an Address value, ensure ABI/serialization
encodes escrow_contract as an Address type (not string), and rename any
variables or DTO fields from escrow_id -> escrow_contract to match
DeployAllParams (e.g., where deploy_all, build_deploy_params, or similar
functions construct the struct) so the deployment transaction sends an Address
rather than a string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@apps/smart-contracts/contracts/deployer/src/deployer.rs`:
- Around line 20-35: DeployAllParams now expects escrow_contract: Address but
callers still pass a stale escrow_id and encode the escrow as a string; update
every call site that constructs DeployAllParams (and any helper builders) to
stop using escrow_id and instead pass the escrow_contract as an Address value,
ensure ABI/serialization encodes escrow_contract as an Address type (not
string), and rename any variables or DTO fields from escrow_id ->
escrow_contract to match DeployAllParams (e.g., where deploy_all,
build_deploy_params, or similar functions construct the struct) so the
deployment transaction sends an Address rather than a string.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c0223359-1c7e-44b2-bad2-439923a8b1ae

📥 Commits

Reviewing files that changed from the base of the PR and between 490a3ff and 06a9f6c.

📒 Files selected for processing (8)
  • apps/smart-contracts/contracts/deployer/src/deployer.rs
  • apps/smart-contracts/contracts/deployer/src/test.rs
  • apps/smart-contracts/contracts/participation-token/src/contract.rs
  • apps/smart-contracts/contracts/participation-token/src/metadata.rs
  • apps/smart-contracts/contracts/participation-token/src/storage_types.rs
  • apps/smart-contracts/contracts/participation-token/src/test.rs
  • apps/smart-contracts/contracts/token-sale/src/test.rs
  • apps/smart-contracts/contracts/vault-contract/src/test.rs

@armandocodecr armandocodecr merged commit 647d66c into develop Mar 13, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants