Skip to content
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

Murisi/unconstrained transfers #3459

Merged
merged 19 commits into from
Jul 5, 2024
Merged

Conversation

murisi
Copy link
Contributor

@murisi murisi commented Jun 28, 2024

Describe your changes

An attempt to defragment the inputs and outputs of a transfer to ease the printing of transfers on hardware wallets. More specifically, the following changes have been made:

  • The inputs and outputs of transfers have been vectorized individually as opposed to vectorizing the whole transfer object. So the number of inputs to a transfer does not have to match the number of outputs
  • Inputs and outputs to a transfer no longer have to occur in balancing pairs where the amount taken from a source in a pair has to be fully transferred to the target in the pair
  • The code for constructing a transfer automatically aggregates multiple credits or debits to an account instead of leaving them as separate entries. This also helps with more concise hardware wallet printing
  • Introduced a multi_transfer function in the transaction prelude that can apply any number of balance changes simultaneously instead of just 2 like the existing transfer function
  • Generalized the Transfer event emitted by some operations to allow multiple sources and multiple targets. This involved writing some serialization and deserialisation functions to support the conversions of these events.

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

#3446

Checklist before merging to draft

  • I have added a changelog
  • Git history is in acceptable state

Copy link

codecov bot commented Jun 28, 2024

Codecov Report

Attention: Patch coverage is 10.41257% with 912 lines in your changes missing coverage. Please review.

Project coverage is 53.86%. Comparing base (879a326) to head (98c88ec).
Report is 4 commits behind head on main.

Files Patch % Lines
crates/sdk/src/tx.rs 0.00% 276 Missing ⚠️
crates/sdk/src/masp.rs 0.00% 217 Missing ⚠️
crates/token/src/lib.rs 0.00% 83 Missing ⚠️
crates/apps_lib/src/cli.rs 0.00% 68 Missing ⚠️
crates/node/src/bench_utils.rs 0.00% 46 Missing ⚠️
crates/sdk/src/signing.rs 0.00% 44 Missing ⚠️
crates/trans_token/src/storage.rs 0.00% 44 Missing ⚠️
crates/tx_prelude/src/token.rs 24.00% 38 Missing ⚠️
crates/sdk/src/lib.rs 0.00% 24 Missing ⚠️
crates/node/src/shell/governance.rs 4.34% 22 Missing ⚠️
... and 5 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3459      +/-   ##
==========================================
- Coverage   53.92%   53.86%   -0.07%     
==========================================
  Files         317      317              
  Lines      107575   107746     +171     
==========================================
+ Hits        58011    58032      +21     
- Misses      49564    49714     +150     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@murisi murisi force-pushed the murisi/unconstrained-transfers branch from 08025d9 to 7d2bdf7 Compare June 28, 2024 15:13
@murisi murisi marked this pull request as ready for review June 28, 2024 18:43
Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conceptual ACK, but this should receive careful review for correct arithmetic.

@murisi murisi force-pushed the murisi/unconstrained-transfers branch from 7d2bdf7 to 424d0f6 Compare June 28, 2024 20:36
crates/token/src/lib.rs Outdated Show resolved Hide resolved
@murisi murisi force-pushed the murisi/unconstrained-transfers branch from 424d0f6 to 67130f8 Compare July 1, 2024 14:38
@murisi murisi force-pushed the murisi/unconstrained-transfers branch 2 times, most recently from 43541f3 to 987253d Compare July 1, 2024 15:17
@murisi murisi force-pushed the murisi/unconstrained-transfers branch from 987253d to 317b3ad Compare July 1, 2024 15:34
This was referenced Jul 1, 2024
@brentstone brentstone mentioned this pull request Jul 2, 2024
@yito88 yito88 mentioned this pull request Jul 2, 2024
2 tasks
@murisi murisi requested review from tzemanovic and grarco July 2, 2024 14:41
tzemanovic
tzemanovic previously approved these changes Jul 2, 2024
@grarco grarco mentioned this pull request Jul 2, 2024
2 tasks
Comment on lines +736 to +753
impl serde::Serialize for UserAccount {
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
where
S: serde::Serializer,
{
serializer.collect_str(&self.to_string())
}
}

impl<'de> Deserialize<'de> for UserAccount {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: Deserializer<'de>,
{
let s = <String as Deserialize>::deserialize(deserializer)?;
FromStr::from_str(&s).map_err(serde::de::Error::custom)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace with namada::core::string_encoding::StringEncoded, which uses Display and FromStr to encode some value with serde

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StringEncoded<UserAccount> that is

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@murisi can we try to address any further items related to this PR in a follow-up PR? This has been merged to draft now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, understood. Thanks.

brentstone added a commit that referenced this pull request Jul 4, 2024
* origin/murisi/unconstrained-transfers:
  Generalized the Transfer event to support reporting multiple account changes.
  Unconstrain transfers and combine transfer amounts.
  Renamed some data structures.
  Added a changelog entry.
  Remove now dead code.
  Subsumed unshielding transfer into generalized transfer.
  Subsumed shielding transfer into generalized transfer.
  Subsumed shielded transfer into generalized transfer.
  Renamed TransparentTransfer to Transfer.
  Generalized the TransparentTransfer to support a shielded action.
@brentstone brentstone merged commit 0b1d42d into main Jul 5, 2024
12 of 19 checks passed
@brentstone brentstone deleted the murisi/unconstrained-transfers branch July 5, 2024 21:16
@murisi murisi mentioned this pull request Jul 11, 2024
5 tasks
@grarco grarco mentioned this pull request Sep 9, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants