Now denominate the fee amount in wrapper headers. #2245

murisi · 2023-12-04T14:26:49Z

Describe your changes

Made changes necessary to enable hardware wallets to first correctly display amounts in different tokens and second sign over them. Specifically, the following changes were made:

Converted WrapperTx::amount_per_gas_unit from Amount to DenominatedAmount to enable the hardware wallet to display amount with correct precision regardless of the token.
Modified the protocol to always align the DenominatedAmount::denom field before extracting the DenominatedAmount::admount field so that hardware wallet users cannot be tricked into signing unrelated amounts.
Added checked high level operations (like add, subtract, and multiply) to DenominatedAmount to reduce the potential for arithmetic mistakes when manipulating the significand and denomination.
Fixed the redacting of Transfer::amount and Transfer::token for fully shielded transactions. Also fixed the bug where Transfer::amount and its denomination did not correspond to Transfer::token.

The following are known omissions from this PR:

Ethereum bridge pool transactions: EthereumBridgePool::amount and EthereumBridgePool::fee_amount should probably be converted from Amount to DenominatedAmount to allow the hardware wallet to display quantities properly, and the protocol should safely convert them back to Amounts.

Indicate on which release or other PRs this topic is based on

Namada v0.27.0

Checklist before merging to `draft`

I have added a changelog
Git history is in acceptable state

…d checked arithmetic operations to DenominatedAmount to catch mistakes.

…pe is attached. Integration tests now work.

grarco · 2023-12-06T16:19:06Z

sdk/src/signing.rs

@@ -544,15 +544,15 @@ pub async fn wrap_tx<'a, N: Namada<'a>>(
                let target = namada_core::types::masp::TransferTarget::Address(
                    fee_payer_address.clone(),
                );
-                let fee_amount = DenominatedAmount {
+                let fee_amount = DenominatedAmount::new(


This variable is only used to feed the next call to gen_shielded_transfer. This second function though, takes a DenominatedAmount but only ever uses the amount field of it. I was wondering if we could maybe change the signature of gen_shielded_transfer to take an Amount instead

grarco

Left a minor comment but looks good from my side!

sug0 · 2023-12-07T09:45:39Z

apps/src/lib/config/genesis/templates.rs

@@ -491,7 +491,7 @@ pub struct EthBridgeParams {
 impl TokenBalances {
    pub fn get(&self, pk: common::PublicKey) -> Option<token::Amount> {
        let pk = StringEncoded { raw: pk };
-        self.0.get(&pk).map(|amt| amt.amount)
+        self.0.get(&pk).map(|amt| amt.amount())


oh what a world of pain. you modified genesis code haha. let's hope the conflicts with #2186 won't be too gnarly

sug0 · 2023-12-07T10:05:30Z

core/src/types/token.rs

+        self.increase_precision(denom)
+            .map_err(storage_api::Error::new)
+            .map(|x| x.amount)


self.scale(...) since you have this method available

sug0 · 2023-12-07T10:14:00Z

core/src/types/token.rs

+    /// Checked multiplication. Returns `None` on overflow.
+    pub fn checked_mul(&self, rhs: DenominatedAmount) -> Option<Self> {
+        let amount = self.amount.checked_mul(rhs.amount)?;
+        let denom = self.denom.0.checked_add(rhs.denom.0)?.into();
+        Some(Self { amount, denom })
+    }


add unit tests for this

sug0 · 2023-12-07T10:14:08Z

core/src/types/token.rs

+    /// Checked subtraction. Returns `None` on overflow.
+    pub fn checked_sub(&self, mut rhs: DenominatedAmount) -> Option<Self> {
+        let mut lhs = *self;
+        if lhs.denom < rhs.denom {
+            lhs = lhs.increase_precision(rhs.denom).ok()?;
+        } else {
+            rhs = rhs.increase_precision(lhs.denom).ok()?;
+        }
+        let amount = lhs.amount.checked_sub(rhs.amount)?;
+        Some(Self {
+            amount,
+            denom: lhs.denom,
+        })
+    }


add unit tests for this

sug0 · 2023-12-07T10:14:16Z

core/src/types/token.rs

+    /// Checked addition. Returns `None` on overflow.
+    pub fn checked_add(&self, mut rhs: DenominatedAmount) -> Option<Self> {
+        let mut lhs = *self;
+        if lhs.denom < rhs.denom {
+            lhs = lhs.increase_precision(rhs.denom).ok()?;
+        } else {
+            rhs = rhs.increase_precision(lhs.denom).ok()?;
+        }
+        let amount = lhs.amount.checked_add(rhs.amount)?;
+        Some(Self {
+            amount,
+            denom: lhs.denom,
+        })
+    }


add unit tests for this

sug0 · 2023-12-07T10:19:56Z

core/src/types/token.rs

 }

 impl DenominatedAmount {
+    /// Make a new denominated amount


I would rework the docstr here to make it a bit more clear that what we're returning is amount * 10^denom

sug0 · 2023-12-07T10:20:09Z

core/src/types/token.rs

 }

 impl DenominatedAmount {
+    /// Make a new denominated amount
+    pub fn new(amount: Amount, denom: Denomination) -> Self {


Suggested change

pub fn new(amount: Amount, denom: Denomination) -> Self {

pub const fn new(amount: Amount, denom: Denomination) -> Self {

sug0 · 2023-12-07T10:21:39Z

core/src/types/token.rs

+    /// Returns the significand of this number
+    pub fn amount(&self) -> Amount {
+        self.amount
+    }


this method might be a bit confusing, since it doesn't specify that the returned amount is not multiplied by the denomination. maybe mention this in the docstr.

also, I think this can become a const fn

sug0 · 2023-12-07T10:21:46Z

core/src/types/token.rs

+    }
+
+    /// Returns the denomination of this number
+    pub fn denom(&self) -> Denomination {


Suggested change

pub fn denom(&self) -> Denomination {

pub const fn denom(&self) -> Denomination {

sug0 · 2023-12-07T10:23:49Z

tests/src/e2e/eth_bridge_tests.rs

+    let amount =
+        token::DenominatedAmount::new(transfer_amount, 0u8.into()).to_string();


I wouldn't bother touching the eth bridge e2e tests, honestly. they're still disabled

…pdate steward commission, and redelegate.

murisi force-pushed the murisi/denominated-fee-amount branch from 0e487a1 to bab08c8 Compare December 4, 2023 15:38

Now denominate the fee amount in wrapper headers.

fd07f6b

murisi force-pushed the murisi/denominated-fee-amount branch from bab08c8 to fd07f6b Compare December 4, 2023 15:55

murisi added 3 commits December 4, 2023 18:18

Removed to_ledger_vectors dependency on Namada client.

32ec88c

Protocol now ensures that a DenominatedAmounts denom is correct. Adde…

439fec1

…d checked arithmetic operations to DenominatedAmount to catch mistakes.

Dont introduce denominations in the bridge pool transaction WASM.

f551e29

murisi force-pushed the murisi/denominated-fee-amount branch from 95c5f6f to f551e29 Compare December 5, 2023 17:03

Fixed the redaction of MASP amounts and ensured that correct token ty…

e8cb0c1

…pe is attached. Integration tests now work.

murisi force-pushed the murisi/denominated-fee-amount branch from 3425622 to e8cb0c1 Compare December 6, 2023 07:10

murisi marked this pull request as ready for review December 6, 2023 08:07

murisi mentioned this pull request Dec 6, 2023

Amounts, denominated amounts, and potential unit errors #2247

Open

murisi requested review from grarco, batconjurer and sug0 December 6, 2023 08:25

grarco reviewed Dec 6, 2023

View reviewed changes

grarco previously approved these changes Dec 6, 2023

View reviewed changes

murisi dismissed grarco’s stale review via 81d5123 December 7, 2023 08:03

murisi force-pushed the murisi/denominated-fee-amount branch from 81d5123 to a9ff48e Compare December 7, 2023 08:55

batconjurer previously approved these changes Dec 7, 2023

View reviewed changes

sug0 requested changes Dec 7, 2023

View reviewed changes

Fixed the formatting of update account in the test vectors.

8b28d27

murisi dismissed batconjurer’s stale review via 8b28d27 December 7, 2023 13:55

murisi force-pushed the murisi/denominated-fee-amount branch from a9ff48e to 8b28d27 Compare December 7, 2023 13:55

anoma deleted a comment from leotese Dec 8, 2023

murisi added 3 commits December 8, 2023 18:56

Added a unit test for denominated arithmetic.

ef1894c

Add test vector printers for bridge pool transfers, resign steward, u…

41acfb9

…pdate steward commission, and redelegate.

Added changelog entry.

ff27346

brentstone merged commit a723d00 into main Dec 29, 2023
12 of 14 checks passed

brentstone deleted the murisi/denominated-fee-amount branch December 29, 2023 19:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Now denominate the fee amount in wrapper headers. #2245

Now denominate the fee amount in wrapper headers. #2245

murisi commented Dec 4, 2023 •

edited

Loading

grarco Dec 6, 2023

grarco left a comment

sug0 Dec 7, 2023

sug0 Dec 7, 2023

sug0 Dec 7, 2023

sug0 Dec 7, 2023

sug0 Dec 7, 2023

sug0 Dec 7, 2023

sug0 Dec 7, 2023

sug0 Dec 7, 2023

sug0 Dec 7, 2023

sug0 Dec 7, 2023

	pub fn new(amount: Amount, denom: Denomination) -> Self {
	pub const fn new(amount: Amount, denom: Denomination) -> Self {

	pub fn denom(&self) -> Denomination {
	pub const fn denom(&self) -> Denomination {

		let amount =
		token::DenominatedAmount::new(transfer_amount, 0u8.into()).to_string();

Now denominate the fee amount in wrapper headers. #2245

Now denominate the fee amount in wrapper headers. #2245

Conversation

murisi commented Dec 4, 2023 • edited Loading

Describe your changes

Indicate on which release or other PRs this topic is based on

Checklist before merging to draft

Choose a reason for hiding this comment

grarco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

murisi commented Dec 4, 2023 •

edited

Loading

Checklist before merging to `draft`