github-jwinterm/grin-wallet
1
0
Fork 0
mirror of https://github.com/jwinterm/grin-wallet.git synced 2026-10-17 07:11:36 +02:00
Code Issues Packages Projects Releases Wiki Activity

clean up contract review findings

Browse Source
This commit is contained in:
wiesche
2026-08-31 23:43:58 +02:00
parent 12b8072d2d
commit dc46cbe73a
9 changed files with 108 additions and 56 deletions
Download Patch File Download Diff File Expand all files Collapse all files
README.md
+3
View File
@@ -29,6 +29,9 @@ To use the command-line wallet, we recommend using the latest release from the [
Full documentation outlining how to use the command line wallet can be found on [Grin's Wiki](https://github.com/mimblewimble/docs/wiki/Wallet-User-Guide)
The experimental contract workflow and its current limitations are described in
[doc/contracts.md](doc/contracts.md).
To build locally install Rust and execute in the project directory:
```
git submodule update --init --recursive
api/src/foreign.rs
+3 -3
View File
@@ -462,7 +462,7 @@ where
// Below is a foreign wrapper around owner calls to 'new' and 'sign' which are only executed
// if this is a receiving contract. This preserves the ability to receive on a foreign interface.
/// TODO
/// Start a receiving contract through the foreign API.
pub fn contract_new(
&self,
keychain_mask: Option<&SecretKey>,
@@ -473,7 +473,7 @@ where
foreign::contract_new(w, keychain_mask, args)
}
/// TODO
/// Sign the receiving side of a contract through the foreign API.
pub fn contract_sign(
&self,
keychain_mask: Option<&SecretKey>,
@@ -485,7 +485,7 @@ where
foreign::contract_sign(w, keychain_mask, args, slate)
}
/// TODO
/// Verify an invoice payment proof against the chain.
pub fn verify_payment_proof_invoice(
&self,
recipient_address: &DalekPublicKey,
api/src/foreign_rpc.rs
-8
View File
@@ -386,14 +386,6 @@ pub fn run_doctest_foreign(
)
.unwrap();
let _ = fs::create_dir_all(test_dir);
let config = initial_setup_wallet(
&ChainTypes::AutomatedTesting,
Some(PathBuf::from(test_dir)),
false,
)
.unwrap();
let mut wallet_proxy: WalletProxy<
DefaultLCProvider<LocalWalletClient, ExtKeychain>,
LocalWalletClient,
api/src/owner_rpc.rs
-8
View File
@@ -2666,14 +2666,6 @@ pub fn run_doctest_owner(
)
.unwrap();
let _ = fs::create_dir_all(test_dir);
let config = initial_setup_wallet(
&ChainTypes::AutomatedTesting,
Some(PathBuf::from(test_dir)),
false,
)
.unwrap();
let mut wallet_proxy: WalletProxy<
DefaultLCProvider<LocalWalletClient, ExtKeychain>,
LocalWalletClient,
controller/src/command.rs
-2
View File
@@ -1818,8 +1818,6 @@ impl ContractSetupArgs {
fn to_api_args(&self) -> Result<ContractSetupArgsAPI, Error> {
let net_change = self.get_net_change()?;
Ok(ContractSetupArgsAPI {
// TODO: num_participants is derived here. It should be an Option and read from the slate.
// Need to check no attack are possible regarding kernel fee contribution.
net_change: net_change,
add_outputs: self.add_outputs,
selection_args: OutputSelectionArgs {
doc/contracts.md
+64
View File
@@ -0,0 +1,64 @@
# Wallet contracts
Contracts use the same flow for standard sends and invoice transactions. Signing is kept
as an explicit step, so both sides can check the transaction before they sign it.
A two-party contract is `new`, `sign`, `sign`. A self-spend is `new`, `sign`. There is no
separate `setup` command. Setup is done by `new`, or by the first `sign` when needed.
The owner API provides all four commands. The Rust foreign API also provides `new` and
`sign` for receiving contracts, but they are not exposed by the foreign JSON-RPC API. The
API does not require manual confirmation, so a wallet using it still needs to show the
transaction and ask the user before signing. The reference CLI does not show the full
inputs, outputs, fees and balance changes yet.
The CLI exchanges Slatepacks. The last `sign` broadcasts the transaction unless
`--no-broadcast` is used.
## Amounts and fees
`--send` and `--receive` are the amount being transferred, before each side pays its own
fee. The sender spends the amount plus its fee. The receiver gets the amount minus its
fee. Each side pays for the inputs and outputs it adds, plus its rounded-up share of the
kernel fee.
CLI amounts are human readable. Values passed to the API, including `make_outputs`, are in
nanogrin. Outputs requested with `make_outputs` are added together with the output needed
to balance that side of the transaction.
Payjoin is used by default. A receiver adds an input when one is available. An empty wallet
can still receive; the fee is then taken from the received output. `--no-payjoin` prevents
the receiver from adding an input and cannot be used together with `--use-inputs`.
Inputs and outputs are normally picked when signing. `--add-outputs` picks and locks them
during `new`, but they are still only added to the slate when signing. `--use-inputs` and
`--make-outputs` limit the selection but do not lock anything early. Open contracts do not
reserve funds against each other, so a late-locked contract can fail when another
transaction has spent the available outputs first.
## View and revoke
`view` reads a slate or Slatepack and shows the participants, signatures, suggested or
agreed amount change, and whether the local transaction is confirmed. It does not show all
inputs, outputs and fees, and it cannot find a contract by id.
`revoke` cancels the local transaction. When the wallet added an input, it returns a
self-spend of that input. The caller still has to post it, and either transaction can win
if the original is already in the mempool. When the wallet added no input there is no
replacement transaction. The replacement does not use a higher fee. An interrupted
`revoke` can be run again.
## Current limitations
* Only one or two participants are supported
* A returned slate is not yet checked against every input and output saved during setup
* Early invoice payment proofs are only available through the API and require the
experimental Slate V5 format. Other proof types are not supported
* Custom fee rates and contract TTLs are not supported
* There are no contract-specific history, lookup or transport commands. Payment proof
memos cannot be set through the contract API or CLI
* If writing the signed transaction file fails, the wallet state has already been saved.
Cancelling the transaction releases the locked inputs
Implementation notes and remaining work are kept in
[`libwallet/src/contract/actions/README.md`](../libwallet/src/contract/actions/README.md).
libwallet/src/contract/actions/README.md
+35 -32
View File
@@ -1,16 +1,8 @@
# Contract actions
### API endpoints
Four owner API methods, each with a matching `grin-wallet contract` subcommand: `new`,
`sign`, `view` and `revoke`. There is no `setup` endpoint; setup is a step rather than a
call, run by `new` for the initiator and by `sign` for the signer if it hasn't happened
yet. So a two party contract is `new`, `sign`, `sign`, and a self-spend is `new`, `sign`.
`view` summarises a slate: participants, signatures so far, the net change it suggests or
the one we already agreed to, and whether the transaction has confirmed. `revoke`
double-spends an input of a contract we signed but that hasn't confirmed, and is
idempotent, so an interrupted revoke can be run again.
The workflow, command behaviour and current limitations are documented in
[`doc/contracts.md`](../../../../doc/contracts.md). This file tracks implementation details
and unfinished work.
### Rust implementation
@@ -29,8 +21,12 @@ contract_utils::save_step(slate, context, ...);
return slate;
```
We only allow contribution of custom inputs/outputs when we're doing the setup phase. Once the setup phase is done,
we no longer allow any customization of inputs. This means that the customization can only happen at contract setup phase which is the first time we see the contract. Additionally, if we customize output selection, we immediately pick the inputs/outputs which means it's an early lock. These are however not added to the slate until we reach the 'sign' phase of the contract. Counterparties don't need to see our inputs/outputs before that. This means we always add inputs/outputs only when we have to and never before.
`save_step` writes the context, tx log, outputs and input locks in one LMDB batch. The
signed transaction is stored separately outside LMDB and cannot share that batch.
Custom input and output selection is only accepted during setup. `add_outputs` selects and
locks them during setup; otherwise this happens during signing. In either case they are
only added to the slate when signing, so the counterparty does not see them earlier.
Ideally we'd also separate side effects out of these functions e.g. computing the current_height
or refreshing the outputs with updater::refresh_outputs(...). The current_height could be
@@ -38,23 +34,15 @@ communicated through a &ChainState parameter which would collect these values be
Additionally, we could fetch the existing Context before the call to avoid doing db fetch.
Separating side effects until the 'save_step' part would make these functions much easier to test.
#### Not supported
- More than two parties. `num_participants` is validated to 1 (self-spend) or 2.
- Payment proof types other than invoice (type 1). A variant that doesn't depend on your
position in the contract is RFC work.
- `--send` and `--receive` take a human readable amount, as `send` and `invoice` do.
Everything reaching the API, including `--make-outputs`, is nanogrin.
- Writing the signed transaction in the same commit as the wallet state. `store_tx`
writes a file outside LMDB, as it does for every transaction in the wallet;
`contract::utils::save_step` describes the window and how to recover.
#### TODOs
- `next_child_for` reads the derivation index and writes it back in two batches, so
concurrent steps can be handed the same child key. `next_child` has the same shape.
- Contract input selection requires a single confirmation.
- `--use-inputs` names the inputs to spend but doesn't lock them, unlike `--add-outputs`.
- Honour `min_input_confirmation`; contract input selection currently uses one confirmation.
- Decide whether `--use-inputs` or `--make-outputs` should imply early locking; currently
only `--add-outputs` locks during `new`.
- Reserve funds across open late-locked contracts, or make the possible signing failure
clearer to callers.
- Check the commitments we contributed are the ones that come back, so a counterparty
can't return a slate carrying inputs or outputs of ours that we didn't put there.
- Handle the coinbase flows. Coinbase inputs are built into the slate, but nothing else
@@ -62,20 +50,37 @@ Separating side effects until the 'save_step' part would make these functions mu
- For a payjoin, `--use-inputs any` picks an input for us; naming the commitment would be
clearer, for the API as much as the CLI.
- `new` has no `--no-setup`.
- Detect a reversed `--send` or `--receive` when signing a slate for the first time.
- Let `view` report whether a slatepack was encrypted for this wallet.
- Show all inputs, outputs, fees and resulting balance changes before the CLI signs.
- Distinguish the transfer amount from the fee-adjusted balance change in `view`.
- Raise the replacement fee when `revoke` races a transaction already in the mempool.
- Merge invoice-proof retrieval with the existing payment-proof API.
- Confirm that invoice-proof retrieval excludes the transaction fee from the amount.
- Move payment-proof creation and verification onto `Slate` so it is versioned with the
slate format.
- Honour `ProofArgs::proof_type`; contract proofs currently always use the invoice type.
- Clean up Slatepack output: return valid JSON after broadcasting, choose the sender
derivation path explicitly, and keep encrypted outgoing files identifiable locally.
- Decode the sender and slate from an incoming Slatepack in one pass.
- Decide whether `target_slate_version` should control Slatepack output or be removed.
- Add contract history, lookup by id, transport support and configurable proof memos.
- Add full API and RPC examples once the contract interface is stable.
- Separate side effects out from the computation, as described above.
- Decide whether `contract_accounts_switch.rs` is a legitimate scenario, or whether
signing under a different account than setup should error.
- Decide whether `target_slate_version` should control Slatepack output or be removed.
#### Tests
- signing the same slate twice, which `verify_not_signed` guards
- zero-value outputs, and `--make-outputs` beyond what is available
- a receiver with no spendable input, through lack of funds or of confirmations
- contracts spending more than one input
- invalid or inconsistent slates, including changed commitments and unexpected states
- the CLI conflict between `--no-payjoin` and `--use-inputs`
- revoke when different accounts have the same numeric transaction id
- a committed fee that no longer covers the selected inputs and outputs
- slate contents at each step, not only the end state
- the foreign API entry points for `new` and `sign`
- fee contribution across one and two parties
#### save_step
@@ -91,9 +96,7 @@ Separating side effects until the 'save_step' part would make these functions mu
// - maybe_context = w.get_private_context(keychain_mask, sl.id.as_bytes())
// - create_contract_ctx -> updater::refresh_outputs(wallet, keychain_mask, parent_key_id, false)?;
// - add_outputs -> let current_height = w.w2n_client().get_chain_tip()?.0;
// - add_outputs -> contribute_output -> let key_id = keys::next_available_key(wallet, keychain_mask).unwrap();
// - TODO: would we need to compute keys::next_available_key for as many outputs as we plan to contribute and pass
// them as a param to keep this without side effects?
// - add_outputs -> add_outputs_to_ctx -> w.next_child_for(...)
#### Sign
// Side-effects:
libwallet/src/contract/context.rs
+1 -1
View File
@@ -127,7 +127,7 @@ where
// Context.amount is not used in contracts, but we set it anyway.
context.amount = slate.amount;
// Store the full setup args on the context; later steps (add_outputs) read the
// SelectionArgs and ttl from here.
// selection arguments from here.
context.setup_args = Some(setup_args.clone());
debug!(
"Setting Context.net_change as: {}",
libwallet/src/contract/types.rs
+2 -2
View File
@@ -84,7 +84,7 @@ impl Default for OutputSelectionArgs {
pub enum ProofType {
/// Legacy (0x00)
Legacy,
/// Invoice (0x01, Default)
/// Invoice (0x01)
Invoice,
/// Sender Nonce (0x02)
SenderNonce,
@@ -95,7 +95,7 @@ pub enum ProofType {
pub struct ProofArgs {
/// If net change is positive during this step, whether to suppress the creation of payment proof
pub suppress_proof: bool,
/// Type of proof (Default 'Invoice')
/// Requested proof type. Contract proofs currently always use the invoice type.
pub proof_type: ProofType,
/// Sender address (required at some stage, may not necessarily be in slate so can be provided explicitly)
#[serde(with = "dalek_ser::option_dalek_pubkey_serde")]