Skip to content
This repository has been archived by the owner on Jul 22, 2024. It is now read-only.

fix: change order of arguments in fee transfer's calldata #783

Merged
merged 8 commits into from
Jul 7, 2023

Conversation

MegaRedHand
Copy link
Contributor

Closes #780

Description

We were passing the Uint256 arguments in the fee transfers in big-endian order (most significative first) when in Cairo a Uint256 assumes a little-endian order:

// Represents an integer in the range [0, 2^256).
struct Uint256 {
    // The low 128 bits of the value.
    low: felt,
    // The high 128 bits of the value.
    high: felt,
}

This caused us to try a transfer for the original fee amount times 2**128, which in practice means a failed transaction every time.

This wasn't caught in integration tests, because they all use a gas price of 0.

This PR fixes this issue, modifies tests to use a gas price of 1, and changes error handling, so these kinds of errors can be more easily debugged.

Checklist

  • Linked to Github Issue
  • Unit tests added
  • Integration tests added.
  • This change requires new documentation.
    • Documentation has been added/updated.

@MegaRedHand MegaRedHand added the bug Something isn't working label Jul 6, 2023
@MegaRedHand MegaRedHand self-assigned this Jul 6, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #783 (96d88fa) into main (996bb68) will decrease coverage by 0.02%.
The diff coverage is 97.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #783      +/-   ##
==========================================
- Coverage   92.47%   92.45%   -0.02%     
==========================================
  Files          51       51              
  Lines       12049    12050       +1     
==========================================
- Hits        11142    11141       -1     
- Misses        907      909       +2     
Impacted Files Coverage Δ
src/transaction/declare.rs 98.75% <ø> (-0.15%) ⬇️
src/transaction/error.rs 100.00% <ø> (ø)
src/transaction/invoke_function.rs 99.19% <96.15%> (-0.12%) ⬇️
src/transaction/fee.rs 98.78% <100.00%> (+0.01%) ⬆️
src/utils.rs 94.31% <100.00%> (+0.03%) ⬆️

@matias-gonz matias-gonz added this pull request to the merge queue Jul 7, 2023
Merged via the queue into main with commit a8650ac Jul 7, 2023
@MegaRedHand MegaRedHand deleted the fix-fee-transfer-failure branch July 7, 2023 13:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate FeeTransfer failure when having enough max_fee
4 participants