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

Fixe underflow uint256 sub #146

Merged
merged 5 commits into from
Oct 11, 2022
Merged

Fixe underflow uint256 sub #146

merged 5 commits into from
Oct 11, 2022

Conversation

qperrot
Copy link
Contributor

@qperrot qperrot commented Oct 4, 2022

No description provided.

@archseer
Copy link
Collaborator

archseer commented Oct 4, 2022

link_available_for_payment should also be fixed?

@qperrot qperrot closed this Oct 5, 2022
@qperrot qperrot deleted the fixe-underflow-uint256_sub branch October 5, 2022 07:37
@qperrot qperrot restored the fixe-underflow-uint256_sub branch October 5, 2022 07:38
@qperrot qperrot reopened this Oct 5, 2022
Copy link
Collaborator

@krebernisak krebernisak left a comment

Choose a reason for hiding this comment

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

For the link_available_for_payment fn, if the result is expected to be negative number in situations of contract debt, then there is no issue but this should be properly documented:

However, since the felt type in Cairo is not signed, whoever calls this function will have to interpret the result as the correct negative value.

@qperrot please add in-line docs to the function, and also the line where the subtraction happens so we explain what's expected here.

@@ -1001,6 +1002,11 @@ func withdraw_funds{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_chec
)

let (link_due_uint256 : Uint256) = felt_to_uint256(link_due)
let (res) = uint256_le(link_due_uint256, balance)
with_attr error_message("Total amount due exceeds the balance"):
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should prefix all error messages with contract/namespace like so:

Suggested change
with_attr error_message("Total amount due exceeds the balance"):
with_attr error_message("Aggregator: Total amount due exceeds the balance"):

Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to do in this PR. I've created a separate issue for it: #156

@archseer archseer temporarily deployed to integration October 11, 2022 03:08 Inactive
@archseer archseer temporarily deployed to integration October 11, 2022 03:27 Inactive
@archseer archseer temporarily deployed to integration October 11, 2022 03:32 Inactive
@github-actions
Copy link

Smoke Test Results

1 tests   1 ✔️  7m 1s ⏱️
1 suites  0 💤
1 files    0

Results for commit fe33bfc.

@krebernisak krebernisak merged commit f54730f into develop Oct 11, 2022
@krebernisak krebernisak deleted the fixe-underflow-uint256_sub branch October 11, 2022 07:05
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.

3 participants