Skip to content

Commit 9558440

Browse files
authored
Merge branch 'main' into warn-overflow-checks
2 parents 650eb24 + f62c04d commit 9558440

File tree

7 files changed

+84
-46
lines changed

7 files changed

+84
-46
lines changed

.github/workflows/binaries.yml

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ jobs:
3535
target: aarch64-unknown-linux-gnu
3636
runs-on: ${{ matrix.sys.os }}
3737
steps:
38-
- uses: actions/checkout@v5
38+
- uses: actions/checkout@v6
3939
- uses: ./.github/actions/build-binary
4040
with:
4141
target: ${{ matrix.sys.target }}
@@ -59,7 +59,7 @@ jobs:
5959
target: x86_64-apple-darwin
6060
runs-on: ${{ matrix.sys.os }}
6161
steps:
62-
- uses: actions/checkout@v5
62+
- uses: actions/checkout@v6
6363
- uses: ./.github/actions/build-binary
6464
with:
6565
target: ${{ matrix.sys.target }}
@@ -82,7 +82,7 @@ jobs:
8282
ext: .exe
8383
runs-on: ${{ matrix.sys.os }}
8484
steps:
85-
- uses: actions/checkout@v5
85+
- uses: actions/checkout@v6
8686
- uses: ./.github/actions/build-binary
8787
with:
8888
target: ${{ matrix.sys.target }}
@@ -94,7 +94,7 @@ jobs:
9494
needs: [build, build-macos, build-windows]
9595
runs-on: windows-latest
9696
steps:
97-
- uses: actions/checkout@v5
97+
- uses: actions/checkout@v6
9898

9999
- name: Setup vars
100100
run: |
@@ -107,7 +107,7 @@ jobs:
107107
echo "SM_CLIENT_CERT_FILE=D:\\sm_client_cert.p12" >> "$GITHUB_ENV"
108108
109109
- name: Download Artifact
110-
uses: actions/download-artifact@v5
110+
uses: actions/download-artifact@v7
111111
with:
112112
name: ${{ env.ARTIFACT_NAME }}
113113

.github/workflows/bindings-ts.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,10 +27,10 @@ jobs:
2727
- uses: stellar/quickstart@main
2828
with:
2929
tag: testing
30-
- uses: actions/setup-node@v5
30+
- uses: actions/setup-node@v6
3131
with:
3232
node-version: "20.x"
33-
- uses: actions/checkout@v5
33+
- uses: actions/checkout@v6
3434
- uses: stellar/actions/rust-cache@main
3535
- run: rustup update
3636
- name: install optional dependencies (Linux only)

.github/workflows/ledger-emulator.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ jobs:
3434
CI_TESTS: true
3535
TEST_THREADS: ${{ contains(matrix.sys, 'macos') && '--test-threads=1' || '' }} # macOS has limited resources, so we run tests (that rely on `testcontainers`) with 1 thread
3636
steps:
37-
- uses: actions/checkout@v5
37+
- uses: actions/checkout@v6
3838

3939
- uses: stellar/actions/rust-cache@main
4040

.github/workflows/rpc-tests.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ jobs:
3434
- uses: stellar/quickstart@main
3535
with:
3636
tag: future
37-
- uses: actions/checkout@v5
37+
- uses: actions/checkout@v6
3838
- uses: stellar/actions/rust-cache@main
3939
- run: rustup update
4040
- run:

.github/workflows/rust.yml

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -51,15 +51,15 @@ jobs:
5151
check: [advisories, bans, licenses, sources]
5252
continue-on-error: ${{ matrix.check == 'advisories' }}
5353
steps:
54-
- uses: actions/checkout@v5
55-
- uses: EmbarkStudios/cargo-deny-action@f2ba7abc2abebaf185c833c3961145a3c275caad
54+
- uses: actions/checkout@v6
55+
- uses: EmbarkStudios/cargo-deny-action@3fd3802e88374d3fe9159b834c7714ec57d6c979
5656
with:
5757
command: check ${{ matrix.check }}
5858

5959
check:
6060
runs-on: ubuntu-latest-8-cores
6161
steps:
62-
- uses: actions/checkout@v5
62+
- uses: actions/checkout@v6
6363
- uses: stellar/actions/rust-cache@main
6464
- run: sudo apt update && sudo apt install -y libudev-dev libdbus-1-dev
6565
- run: rustup update
@@ -79,7 +79,7 @@ jobs:
7979
target: aarch64-unknown-linux-gnu
8080
runs-on: ${{ matrix.sys.os }}
8181
steps:
82-
- uses: actions/checkout@v5
82+
- uses: actions/checkout@v6
8383
- uses: ./.github/actions/build-and-test
8484
with:
8585
target: ${{ matrix.sys.target }}
@@ -98,7 +98,7 @@ jobs:
9898
target: aarch64-apple-darwin
9999
runs-on: ${{ matrix.sys.os }}
100100
steps:
101-
- uses: actions/checkout@v5
101+
- uses: actions/checkout@v6
102102
- uses: ./.github/actions/build-and-test
103103
with:
104104
target: ${{ matrix.sys.target }}
@@ -115,7 +115,7 @@ jobs:
115115
target: x86_64-pc-windows-msvc
116116
runs-on: ${{ matrix.sys.os }}
117117
steps:
118-
- uses: actions/checkout@v5
118+
- uses: actions/checkout@v6
119119
- uses: ./.github/actions/build-and-test
120120
with:
121121
target: ${{ matrix.sys.target }}

cmd/soroban-cli/src/assembled.rs

Lines changed: 67 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -11,13 +11,11 @@ use soroban_rpc::{
1111
Error, LogEvents, LogResources, ResourceConfig, RestorePreamble, SimulateTransactionResponse,
1212
};
1313

14-
pub(crate) const DEFAULT_TRANSACTION_FEES: u32 = 100;
15-
1614
pub async fn simulate_and_assemble_transaction(
1715
client: &soroban_rpc::Client,
1816
tx: &Transaction,
1917
resource_config: Option<ResourceConfig>,
20-
resource_fee: Option<u64>,
18+
resource_fee: Option<i64>,
2119
) -> Result<Assembled, Error> {
2220
let envelope = TransactionEnvelope::Tx(TransactionV1Envelope {
2321
tx: tx.clone(),
@@ -64,7 +62,7 @@ impl Assembled {
6462
pub fn new(
6563
txn: &Transaction,
6664
sim_res: SimulateTransactionResponse,
67-
resource_fee: Option<u64>,
65+
resource_fee: Option<i64>,
6866
) -> Result<Self, Error> {
6967
let txn = assemble(txn, &sim_res, resource_fee)?;
7068
Ok(Self { txn, sim_res })
@@ -205,7 +203,7 @@ impl Assembled {
205203
fn assemble(
206204
raw: &Transaction,
207205
simulation: &SimulateTransactionResponse,
208-
resource_fee: Option<u64>,
206+
resource_fee: Option<i64>,
209207
) -> Result<Transaction, Error> {
210208
let mut tx = raw.clone();
211209

@@ -219,16 +217,26 @@ fn assemble(
219217
});
220218
}
221219

222-
let min_resource_fee = if let Some(rf) = resource_fee {
223-
tracing::trace!(
224-
"setting resource fee to {rf} from {}",
225-
simulation.min_resource_fee
226-
);
227-
rf
228-
} else {
229-
simulation.min_resource_fee
220+
let mut transaction_data = simulation.transaction_data()?;
221+
let min_resource_fee = match resource_fee {
222+
Some(rf) => {
223+
tracing::trace!(
224+
"overriding resource fee to {rf} (simulation suggested {})",
225+
simulation.min_resource_fee
226+
);
227+
transaction_data.resource_fee = rf;
228+
// short circuit the submission error if the resource fee is negative
229+
// technically, a negative resource fee is valid XDR so it won't panic earlier
230+
// this should not occur as we validate resource fee before calling assemble
231+
u64::try_from(rf).map_err(|_| {
232+
Error::TransactionSubmissionFailed(String::from(
233+
"TxMalformed - negative resource fee",
234+
))
235+
})?
236+
}
237+
// transaction_data is already set from simulation response
238+
None => simulation.min_resource_fee,
230239
};
231-
let transaction_data = simulation.transaction_data()?;
232240

233241
let mut op = tx.operations[0].clone();
234242
if let OperationBody::InvokeHostFunction(ref mut body) = &mut op.body {
@@ -257,13 +265,10 @@ fn assemble(
257265
}
258266
}
259267

260-
// Update transaction fees to meet the minimum resource fees.
261-
// Choose larger of existing fee or inclusion + resource fee.
262-
let min_tx_fee: u64 = DEFAULT_TRANSACTION_FEES.into();
263-
tx.fee = tx.fee.max(
264-
u32::try_from(min_tx_fee + min_resource_fee)
265-
.map_err(|_| Error::LargeFee(min_tx_fee + min_resource_fee))?,
266-
);
268+
// Update the transaction fee to be the sum of the inclusion fee and the
269+
// minimum resource fee from simulation.
270+
let total_fee: u64 = u64::from(raw.fee) + min_resource_fee;
271+
tx.fee = u32::try_from(total_fee).map_err(|_| Error::LargeFee(total_fee))?;
267272

268273
tx.operations = vec![op].try_into()?;
269274
tx.ext = TransactionExt::V1(transaction_data);
@@ -519,6 +524,22 @@ mod tests {
519524
}
520525
}
521526

527+
#[test]
528+
fn test_assemble_transaction_calcs_fee() {
529+
let mut sim = simulation_response();
530+
sim.min_resource_fee = 12345;
531+
let mut txn = single_contract_fn_transaction();
532+
txn.fee = 10000;
533+
let Ok(result) = assemble(&txn, &sim, None) else {
534+
panic!("assemble failed");
535+
};
536+
537+
assert_eq!(12345 + 10000, result.fee);
538+
// validate it updated sorobantransactiondata block in the tx ext
539+
let expected_tx_data = transaction_data();
540+
assert_eq!(TransactionExt::V1(expected_tx_data), result.ext);
541+
}
542+
522543
#[test]
523544
fn test_assemble_transaction_overflow_behavior() {
524545
//
@@ -561,18 +582,35 @@ mod tests {
561582
#[test]
562583
fn test_assemble_transaction_with_resource_fee() {
563584
let sim = simulation_response();
564-
let txn = single_contract_fn_transaction();
565-
let resource_fee = 12345u64;
585+
let mut txn = single_contract_fn_transaction();
586+
txn.fee = 500;
587+
let resource_fee = 12345i64;
566588
let Ok(result) = assemble(&txn, &sim, Some(resource_fee)) else {
567589
panic!("assemble failed");
568590
};
569591

570-
// validate it auto updated the tx fees from sim response fees
571-
// since it was greater than tx.fee
572-
assert_eq!(12345 + 100, result.fee);
592+
// validate the assembled tx fee is the sum of the inclusion fee (txn.fee)
593+
// and the resource fee
594+
assert_eq!(12345 + 500, result.fee);
573595

574596
// validate it updated sorobantransactiondata block in the tx ext
575-
assert_eq!(TransactionExt::V1(transaction_data()), result.ext);
597+
let mut expected_tx_data = transaction_data();
598+
expected_tx_data.resource_fee = resource_fee;
599+
assert_eq!(TransactionExt::V1(expected_tx_data), result.ext);
600+
}
601+
602+
// This should never occur, as resource fee is validated before being passed into
603+
// assemble. But test the behavior just in case.
604+
#[test]
605+
fn test_assemble_transaction_input_resource_fee_negative_errors() {
606+
let mut sim = simulation_response();
607+
sim.min_resource_fee = 12345;
608+
let mut txn = single_contract_fn_transaction();
609+
txn.fee = 500;
610+
let resource_fee = -1;
611+
let result = assemble(&txn, &sim, Some(resource_fee));
612+
613+
assert!(result.is_err());
576614
}
577615

578616
#[test]
@@ -592,7 +630,7 @@ mod tests {
592630
assert_eq!(txn.fee, 100, "modified txn.fee: update the math below");
593631

594632
// 1: wiggle room math overflows but result fits
595-
let resource_fee: u64 = (u32::MAX - 100).into();
633+
let resource_fee: i64 = (u32::MAX - 100).into();
596634

597635
match assemble(&txn, &response, Some(resource_fee)) {
598636
Ok(asstxn) => {
@@ -603,7 +641,7 @@ mod tests {
603641
}
604642

605643
// 2: combo overflows, should throw
606-
let resource_fee: u64 = (u32::MAX - 99).into();
644+
let resource_fee: i64 = (u32::MAX - 99).into();
607645

608646
match assemble(&txn, &response, Some(resource_fee)) {
609647
Err(Error::LargeFee(fee)) => {

cmd/soroban-cli/src/resources.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,8 @@ use crate::commands::HEADING_RPC;
1111
#[group(skip)]
1212
pub struct Args {
1313
/// Set the fee for smart contract resource consumption, in stroops. 1 stroop = 0.0000001 xlm. Overrides the simulated resource fee
14-
#[arg(long, env = "STELLAR_RESOURCE_FEE", help_heading = HEADING_RPC)]
15-
pub resource_fee: Option<u64>,
14+
#[arg(long, env = "STELLAR_RESOURCE_FEE", value_parser = clap::value_parser!(i64).range(0..u32::MAX.into()), help_heading = HEADING_RPC)]
15+
pub resource_fee: Option<i64>,
1616
/// ⚠️ Deprecated, use `--instruction-leeway` to increase instructions. Number of instructions to allocate for the transaction
1717
#[arg(long, help_heading = HEADING_RPC)]
1818
pub instructions: Option<u32>,

0 commit comments

Comments
 (0)