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

fix!: assembly process for account transfer operation #2963

Merged
merged 12 commits into from
Aug 18, 2024

Conversation

Torres-ssf
Copy link
Contributor

@Torres-ssf Torres-ssf commented Aug 16, 2024

Summary

This PR improves the logic for assembling account transfer operations by extracting the sender's address directly from the transaction inputs (Coins and Messages) instead of relying on the OutputChange. This update ensures the sender's identification is more accurate in account transfer operations.

Breaking Changes

The getTransferOperations helper function now requires an additional baseAssetId parameter.

// before
const transferOperations = getTransferOperations({ inputs, outputs, receipts })
// after
const transferOperations = getTransferOperations({ inputs, outputs, receipts, baseAssetId })

Checklist

  • All changes are covered by tests (or not applicable)
  • All changes are documented (or not applicable)
  • I reviewed the entire PR myself (preferably, on GH UI)
  • I described all Breaking Changes (or there's none)

@Torres-ssf Torres-ssf changed the title fix!: assembly process for account transfer operation fix: assembly process for account transfer operation Aug 16, 2024
Copy link
Contributor

@maschad maschad left a comment

Choose a reason for hiding this comment

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

Nice work on this @Torres-ssf , LGTM I've just left a comment re: release notes.

I've tested this out on fuel-wallet and verified the fix as well.

Copy link
Contributor

@petertonysmith94 petertonysmith94 left a comment

Choose a reason for hiding this comment

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

Hey @Torres-ssf,

I added a PR with two tests - one of which didn't have the results I was expected. If you could assist my understand, I'd be grateful.

@maschad maschad changed the title fix: assembly process for account transfer operation fix!: assembly process for account transfer operation Aug 17, 2024
maschad
maschad previously approved these changes Aug 17, 2024
@maschad maschad requested review from maschad and arboleya August 17, 2024 19:11
Copy link
Contributor

Coverage Report:

Lines Branches Functions Statements
79.19%(+0.06%) 71.47%(+0.02%) 77.31%(+0.03%) 79.32%(+0.06%)
Changed Files:
Ok File (✨=New File) Lines Branches Functions Statements
🔴 packages/account/src/providers/transaction-summary/input.ts 97.67%
(+1.25%)
95.65%
(+2.32%)
100%
(+0%)
97.82%
(+1.05%)
🔴 packages/account/src/providers/transaction-summary/operations.ts 95.79%
(+0.26%)
71.21%
(-2.92%)
93.47%
(-0.14%)
94.44%
(+0.28%)

@Torres-ssf Torres-ssf merged commit 80cb187 into master Aug 18, 2024
20 checks passed
@Torres-ssf Torres-ssf deleted the st/fix/account-transfer-operation branch August 18, 2024 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transaction Summary showing incorrect operations for faucet transaction
6 participants