Repository navigation
fix(sweep_controller): remove token transfer from sweep_account to match the SDK's custodial payout model - #760
Merged
Conversation
…DK custodial payout sweep_account called transfers::execute_transfers after EphemeralAccount::sweep, which invoked TokenClient::transfer(from = <ephemeral contract>, ...). That can never succeed: a contract must authorize its own outgoing transfer (sweep() never does) and the funds sit on the ephemeral account's classic G... address, not on the contract, so a fixed authorization would still leave no balance to move. It would also double-pay the SDK's classic Horizon payout. Remove the transfer call and the now-unused transfers module; the controller verifies the signature/nonce, authorizes and marks the account swept, and emits SweepCompleted. Update integration tests to exercise the sweep path with explicit mock_auths only (no blanket mock_all_auths) and assert a missing controller authorization fails. Align docs/api-reference.md, docs/architecture.md and the README with the custodial payout model. Refs #758
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
sweep_accountfinished by callingtransfers::execute_transfers, which didTokenClient::transfer(from = <ephemeral contract>, to = destination, amount). That can never succeed on-chain:authorize_as_current_contract) andEphemeralAccount::sweep()never does, so the host rejects it (Error(Auth, InvalidAction)).G...account via Horizon; the contract only records the payment (record_payment). Even with the auth fixed there is no balance to move.SweepsServicealready performs the classic Horizon payout.The transfer was added in
b21f6bcand the integration tests missed it because they usedmock_all_auths(). This PR removes the transfer so the controller matches the SDK's custodial payout model: it verifies the signature/nonce, authorizes and marks the account swept, and emitsSweepCompleted. It moves no tokens.Changes
contracts/sweep_controller/src/lib.rs- drop theexecute_transferscall and thePaymentconversion insweep_account; removemod transfers;and the now-unusedPaymentimport; fix theexecute_sweepdoc comment.contracts/sweep_controller/src/transfers.rs- deleted (only used bysweep_account).contracts/sweep_controller/tests/integration.rs- exercise the sweep path with explicitmock_authsonly, no blanketmock_all_auths; add a test asserting a missing controller authorization fails.docs/api-reference.md,docs/architecture.md,README.md- document that the controller does not move funds and the SDK performs the custodial payout.Tests
Validated locally that a successful
execute_sweep(with a real Ed25519 signature overSHA256(destination || nonce || controller)) now passes, and that the same path fails attransferon the pre-change code. Nomock_all_auths()was added; the suite does not rely on one.cargo fmt --all -- --check- cleancargo clippy -p sweep_controller --lib -- -D warnings- cleancargo test- all pass (ephemeral_account32,reserve_contract20,sweep_controllerintegration 9)cargo build --release --target wasm32v1-none- all four WASMs buildNo new dependencies
The success-path test that requires signing was run during development and then removed so the PR introduces no dependency. The committed tests cover the sweep path with explicit auths only.
Redeploy steps (new controller ID pending)
The live controller
CBEU4X5M...Z5KWEstill contains the transfer call. Redeploy is required and must be run by a key-holder (no deployer secret available here):stellar contract buildstellar contract deploy --wasm target/wasmv1-none/release/sweep_controller.wasm --network testnet --source <DEPLOYER_SECRET>stellar contract invoke --id <NEW_CONTROLLER_ID> --network testnet --source <DEPLOYER_SECRET> -- initialize --creator <CREATOR> --authorized_signer 16ac79d642e33ac696e822cc7175ad7ddd5587a9dd8b6942640cbb832a1ab4bf --authorized_destination <none|DEST>deployments/testnet.jsonassweepController.Existing test accounts bound to the old controller keep pointing at the old
authorized_controller; they need fresh sends against accounts initialized with the new controller ID.Refs #758