Skip to content

Fix outdated Address and token APIs in EVM deployment migration guide - #2689

Open
leighmcculloch wants to merge 2 commits into
stellar:mainfrom
leighmcculloch:fix-fabricated-apis-evm-deployment-migration
Open

Fix outdated Address and token APIs in EVM deployment migration guide#2689
leighmcculloch wants to merge 2 commits into
stellar:mainfrom
leighmcculloch:fix-fabricated-apis-evm-deployment-migration

Conversation

@leighmcculloch

Copy link
Copy Markdown
Member

What

Replace the removed Address::random call with Address::generate, update the quoted token-interface approve parameter name, bump the pinned soroban-sdk version, and point the example clone at a current ref.

Why

Address::random no longer exists in the SDK and the quoted parameter name was renamed, so a reader following this migration guide would hit compile errors and copy an incorrect interface.

### What
Replace the removed `Address::random` call with `Address::generate`, update the quoted token-interface `approve` parameter name, bump the pinned soroban-sdk version, and point the example clone at a current ref.

### Why
`Address::random` no longer exists in the SDK and the quoted parameter name was renamed, so a reader following this migration guide would hit compile errors and copy an incorrect interface.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Updates the EVM migration guide for Soroban SDK 27 and renamed APIs.

Changes:

  • Updates SDK dependencies and Address::generate.
  • Renames the token approval parameter.
  • Changes the examples checkout to main.

Recommendation: NEEDS-CHANGESmain uses a token ABI incompatible with the guide’s initialization and transfer calls.


```bash
git clone -b v22.0.1 https://github.com/stellar/soroban-examples
git clone -b main https://github.com/stellar/soroban-examples

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Addressed in 50fc162, by updating the guide to the current ABI rather than re-pinning to an older ref.

initialize__constructor: create_contract now passes the admin and metadata through deploy_v2, the vault's initialize builds them with String::from_str, and the quoted interface declares __constructor as an inherent function on the contract type (it can't live in a trait impl, and needs no "already initialized" guard). The initialize_token.sh script is gone — its arguments moved onto stellar contract deploy after the --, so the token is deployed and initialized in one transaction. Prose and walkthrough updated to match.

transfer/MuxedAddress: the interface excerpt now declares to_muxed: MuxedAddress and resolves it with .address() before touching balances.

One correction on that second point, though: the call sites at lines 411-416 and 434 do not produce compile errors, so I left them alone. For a MuxedAddress parameter the generated client takes impl Into<MuxedAddress> — see derive_client.rs:213-221, which applies fn_arg_make_into (emitting impl Into<#ty>) and converts with #ident.into().into_val(...). Combined with impl From<&Address> for MuxedAddress, passing &Address compiles fine. The token example's own tests on main do exactly this: token.transfer(&user1, &user2, &600) with two plain Address values.

Also swapped the two stellar contract install invocations for contract upload — the CLI marks install as deprecated.

Not addressed here: the token_interface.rs excerpt is stale beyond transfer (it still declares spendable_balance, authorized, set_authorized, and clawback, which no longer exist upstream, and keeps mint/set_admin inside the interface trait). That's a larger content change and is being tracked separately.

@stellar-jenkins-ci

Copy link
Copy Markdown

Pointing the clone at `main` (previous commit) changes the token contract's
ABI, so the surrounding guide needed to follow:

- The token sets its admin and metadata in `__constructor`; there is no
  `initialize` method. `create_contract` now passes those through
  `deploy_v2`, the vault's `initialize` builds them with `String::from_str`,
  and the quoted interface declares `__constructor` as an inherent function
  on the contract type rather than a trait method (it cannot live in a trait
  impl, and needs no "already initialized" guard).
- The `initialize_token.sh` script is gone. Its arguments move onto
  `stellar contract deploy` after the `--`, so the token is deployed and
  initialized in one transaction.
- `transfer`'s destination is now a `MuxedAddress`, resolved with
  `.address()` before touching balances. Call sites are unchanged: the
  generated client takes `impl Into<MuxedAddress>`, and `MuxedAddress`
  implements `From<&Address>`.
- `stellar contract install` is deprecated in favour of `contract upload`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@ElliotFriend ElliotFriend left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving — your four fixes all check out against upstream.

Fair warning: I pushed a commit of my own (50fc162) that's bigger than what you opened this with. Revert it freely if it over-reaches — your call. Merge if you're happy with it.

Why: Copilot was right that the token contract on main has no initialize (it sets admin and metadata in __constructor), so the guide would dead-end after your clone-ref change. Re-pinning to an older tag would defeat your PR, so I updated the guide instead — deploy_v2 in create_contract, __constructor as an inherent fn in the quoted interface, initialize_token.sh folded into stellar contract deploy, MuxedAddress in the transfer excerpt, and installupload.

Where Copilot was wrong: the transfer call sites are fine as-is. The generated client takes impl Into<MuxedAddress> (derive_client.rs:213-221) and MuxedAddress implements From<&Address> — upstream's own tests call token.transfer(&user1, &user2, &600) with plain Address values.

Left alone deliberately: the token_interface.rs excerpt is stale beyond transfer (spendable_balance, authorized, set_authorized, clawback are all gone upstream). That's ~210 lines and a question about switching to the SDK's token::TokenInterface — deserves its own diff.

Not merging; that's yours.

@stellar-jenkins-ci

Copy link
Copy Markdown

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