-
Notifications
You must be signed in to change notification settings - Fork 53
test(platform-wallet): integration test framework + first transfer test #3549
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: fix/rs-platform-wallet-auto-select-inputs
Are you sure you want to change the base?
Changes from all commits
b5ed6e4
3cf4f06
c7479e6
c3f0faa
e37e60b
c397a38
89d39e7
540bf42
4eb879d
8ac22ee
e064044
a0d50e0
0609acf
ba90e7e
c83bb7f
546be56
276e50a
fe454e2
f86abce
b4d1a6f
7d11975
d394a84
cf8260a
72a9c94
4b46815
b882aa2
ff1a187
dab9285
8ef44a1
20404b3
86f7f04
da98a10
459a61c
854ba2b
142dfed
7b5df76
ed7308c
559eb52
0f4cc68
6223f1e
af714d9
096c15b
5b6acd0
0dd7ec7
ae98ccf
d0b1b96
796f92c
a4696c6
95fc6c2
da73e21
eeeab5e
ffe107c
5515ba9
aad27c5
59cba08
0be256a
74b1ed7
8c7ec00
921833f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -582,6 +582,36 @@ pub struct PlatformAddressChangeSet { | |
| /// Last block height with recent address changes (compaction marker). | ||
| /// `None` means "no change". | ||
| pub last_known_recent_block: Option<u64>, | ||
| /// Lower-bound static fee estimate for the transfer that produced | ||
| /// this changeset, in credits. `0` for changesets not produced by | ||
| /// `transfer()` (e.g. sync-only changesets). See | ||
| /// [`Self::estimated_min_fee`]. | ||
| pub fee: Credits, | ||
| } | ||
|
|
||
| impl PlatformAddressChangeSet { | ||
| /// Lower-bound static fee estimate for the transfer that produced | ||
| /// this changeset, in credits. | ||
| /// | ||
| /// Returns `0` for changesets that didn't originate from a | ||
| /// `transfer()` call — e.g. sync-only changesets, or changesets | ||
| /// constructed via `Default::default()`. The value is the raw | ||
| /// `AddressFundsTransferTransition::estimate_min_fee(input_count, | ||
| /// output_count, version)` result captured at submit time — it is | ||
| /// **NOT** the actual on-chain fee and is **NOT** adjusted by the | ||
| /// `fee_strategy`. | ||
| /// | ||
| /// `estimate_min_fee` only models the static | ||
| /// `state_transition_min_fees` floor; chain-time fees include | ||
| /// storage + processing costs that scale with the operation set | ||
| /// (~6.5M static vs ~14.94M observed real for 1in/1out at the time | ||
| /// of writing). Tests asserting on the actual chain-time debit | ||
| /// must read the post-broadcast balance delta directly, not this | ||
| /// value. See platform issue #3040 for the open ticket on | ||
| /// upgrading `estimate_min_fee` to a chain-time-accurate estimate. | ||
| pub fn estimated_min_fee(&self) -> Credits { | ||
| self.fee | ||
| } | ||
| } | ||
|
|
||
| impl Merge for PlatformAddressChangeSet { | ||
|
|
@@ -606,13 +636,20 @@ impl Merge for PlatformAddressChangeSet { | |
| .map_or(r, |existing| existing.max(r)), | ||
| ); | ||
| } | ||
| // Fee: append-sum via `saturating_add`. Sync-only merges | ||
| // (`fee == 0`) are a no-op so a transfer's recorded fee | ||
| // survives untouched; merging two transfer changesets sums | ||
| // the per-operation fees so the merged total reflects the | ||
| // "total fee paid across operations in this batch" intent. | ||
| self.fee = self.fee.saturating_add(other.fee); | ||
| } | ||
|
Comment on lines
583
to
645
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: PlatformAddressChangeSet::fee is a public Credits field whose own doc admits it does not represent the on-chain fee, and Merge silently sums it Confirmed at HEAD. source: ['claude', 'codex'] 🤖 Fix this with AI agents |
||
|
|
||
| fn is_empty(&self) -> bool { | ||
| self.addresses.is_empty() | ||
| && self.sync_height.is_none() | ||
| && self.sync_timestamp.is_none() | ||
| && self.last_known_recent_block.is_none() | ||
| && self.fee == 0 | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Suggestion:
PlatformAddressChangeSet::feeis a publicCreditsfield that knowingly misrepresents the on-chain fee, andMergesilently sums itpub fee: CreditsstoresAddressFundsTransferTransition::estimate_min_fee(...), which the accessor's own doc admits is "NOT the actual on-chain fee" — the staticstate_transition_min_feesfloor (~6.5M for 1in/1out) is far below the real chain-time debit (~14.94M observed; see #3040). Because the field is public and shares its type with real credit values, callers reaching forcs.fee(rather than the doc-ladenestimated_min_fee()accessor) get the unfiltered footgun.Merge::mergethen doesself.fee = self.fee.saturating_add(other.fee), so a merged changeset's reported fee is "the sum across operations of a number that already lied for one operation," compounding the misrepresentation. Pick one of: (a) wrap in a newtype likeEstimatedMinFee(Credits)so the type system surfaces the caveat at every call site, (b) rename the field tostatic_min_fee_estimateso accidental consumers can't confuse it with paid fees, or (c) keep itpub(crate)until #3040 lands and the value can mean what callers naturally expect.source: ['claude']
🤖 Fix this with AI agents