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!: assemble of Transfer operation #1787

Merged
merged 20 commits into from
Feb 26, 2024
Merged

Conversation

Torres-ssf
Copy link
Contributor

@Torres-ssf Torres-ssf commented Feb 21, 2024

Some details about this PR

  1. Since we already have the OperationName.transfer, I've removed the OperationName.contractTransfer.

    I reckon that we do not need yet another transfer-related operation.

    The Operation object has the from and to properties and each one has a property named type, which can be Account or Contract.

  2. We can simplify transfers in 4 types:

    1. Account to Account - No receipts are generated since no opcodes are called.
    2. Account to Contract - Produces a Transfer receipt.
    3. Contract to Account - Produces a TransferOut receipt.
    4. Contract to Contract - Produces a Transfer receipt.

    This means that the TransferOperation needs to mainly be extracted from Transfer and TransferOut receipts, which was not the case until now.

  3. I've removed the unit tests related to the TransferOperation and replaced them with e2e tests.

    Unit tests involve moving many pieces around and when you find yourself wasting hours creating mocks to cover many specific edge cases, it's because something is wrong.

    Often, these mocks are constructed based on the existing code, which can lead to misleading positive results if the code is wrong. (which was the case until now).

    So I am testing this with e2e tests since it takes fewer steps to simulate each case and we can have more confidence in the test results.

@Torres-ssf Torres-ssf added the bug Issue is a bug label Feb 21, 2024
@Torres-ssf Torres-ssf self-assigned this Feb 21, 2024
Copy link
Member

@Dhaiwat10 Dhaiwat10 left a comment

Choose a reason for hiding this comment

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

First pass - great work!

Dhaiwat10
Dhaiwat10 previously approved these changes Feb 23, 2024
Copy link
Member

@Dhaiwat10 Dhaiwat10 left a comment

Choose a reason for hiding this comment

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

Awesome job @Torres-ssf 👏🏻

Really like the new e2e tests.

apps/docs/src/guide/contracts/variable-outputs.md Outdated Show resolved Hide resolved
danielbate
danielbate previously approved these changes Feb 26, 2024
apps/docs/src/guide/contracts/variable-outputs.md Outdated Show resolved Hide resolved
arboleya
arboleya previously approved these changes Feb 26, 2024
Copy link
Member

@arboleya arboleya left a comment

Choose a reason for hiding this comment

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

Hell of a PR! 🔥

Perhaps we could merge #1801 here.

@Torres-ssf Torres-ssf dismissed stale reviews from arboleya and danielbate via 47e716a February 26, 2024 14:23
Copy link
Contributor

Coverage Report:

Lines Branches Functions Statements
79.42%(-0.04%) 70.17%(-0.15%) 77.9%(-0.02%) 79.5%(-0.04%)
Changed Files:
Ok File (✨=New File) Lines Branches Functions Statements
🔴 packages/account/src/providers/transaction-summary/operations.ts 96.33%
(-1.85%)
74.54%
(-6.59%)
92.1%
(-2.18%)
94.91%
(-2.48%)

@Torres-ssf Torres-ssf merged commit 086dbba into master Feb 26, 2024
15 checks passed
@Torres-ssf Torres-ssf deleted the st/fix/transfer-operations branch February 26, 2024 15:15
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.

Investigate bug on getTransferOperations Improve Variable Outputs documentation page
4 participants