-
Notifications
You must be signed in to change notification settings - Fork 54
test(rs-platform-wallet): e2e suite, Found-025 fix + triage pins #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 59 commits
142dfed
7b5df76
ed7308c
559eb52
0f4cc68
6223f1e
af714d9
096c15b
5b6acd0
0dd7ec7
ae98ccf
d0b1b96
796f92c
a4696c6
95fc6c2
da73e21
eeeab5e
ffe107c
5515ba9
aad27c5
55ad8f9
f54ca47
8f6702d
f09840a
07b56d7
99dcafc
952e605
c89f0ed
81af562
59cba08
930c18a
0be256a
f95b81d
74b1ed7
85cfeb3
2e7c024
99523c2
8a237ae
4b12d1e
98cbfd9
346d404
94902be
8c7ec00
aea4e23
921833f
986f380
2fec68e
bcc5f64
3ee0ce5
dfa39ed
37995e4
a711e5b
c5942d7
3fd4dd9
2dc5d62
5d7ee0c
a717ade
823231c
d7fd62c
173b2e1
93f5306
6352812
c89c631
9997b2e
6471e9e
91941c3
a2dd785
c8b2b5f
386be89
fe6d7c1
26f13d9
bc9a902
d1edc88
d896299
2621e25
835bd80
8325a26
5e365f4
603b444
37b0f76
5dc558d
578a6da
75619fd
75ba17b
458ee80
9df5493
0d17a63
b856fa8
726cee3
3bf4275
fcb1ac3
cbc4302
aead0cc
b9a9293
7760040
717e6c1
4735964
9c62fd8
32ee2cd
9df3aa4
f3416dc
409d088
1a03112
d368436
69ff154
fa55e64
dc186fd
aace4d9
8457aa6
1bd306a
4616cba
23d8943
a3a5d96
a8137a8
1216e5d
a9fed73
41c9493
2c7e22a
97d1532
79843e3
2cd9a7b
391768c
43e3f9d
5d4a61b
b2719d3
f33ba2e
589b5b2
7af098e
cc2104f
6aa4f42
4dd55d2
349b95b
6239fda
4d204cd
25e48dd
e7adc2f
ce94389
5c6baab
f0ea9ad
fcbac27
f6f2702
c03774b
c51836c
8966257
89e8400
81f54b9
2d77385
ab22eff
1e30f63
abc376b
8ae72fd
75f23ea
6e0fab6
1472183
f71183b
7135b4c
c73ccc5
6ac7d62
3d916e3
08bac5e
b4d144b
20ac23c
5f955df
5f1dbda
351d52a
aa70c1e
1c4c8a7
75ac47b
882920a
4583794
26c92c1
1c64b2d
a3903ea
0bd6f7a
ac9d9c4
3411283
1316ab2
9286e1c
1bee06e
822562d
543a8dc
a689c5b
51c1c10
16636f0
f52d268
a55a7a0
caae58c
22cecc1
64d3b6d
ebcddf7
0bacd25
e93c5b3
55472a3
0188fa9
92165ec
5cca0fb
1bbe41c
cf9b6d2
9902cbd
403d29c
fe44be5
55288c6
e9860be
e85d558
b673a7d
0dd5f83
60a2255
6951523
6bb0017
c00f1d4
262ba34
e7e5d6d
02cb61b
63f039a
371e2c3
bd5eb61
926f0a7
ff56c56
fc7e9f8
ce0d555
5466501
e4cf6b3
116bc8a
f209063
dc5e611
ba1e85e
399c40a
f112600
27087f8
d7ed4af
420250d
b01be51
be0899b
6955138
b1d35e4
668a222
63ee3ba
d3c02ca
7651ca8
f625f83
f78deda
f5ddda6
c6693df
435b972
91f1ada
d5fb7f5
1110bd0
0376706
db00fbe
e6bd370
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
626
to
652
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