Skip to content
This repository has been archived by the owner on Sep 5, 2023. It is now read-only.

replace function calls by operators and removed imports and input checks #1041

Conversation

esdras-santos
Copy link
Contributor

This PR do three things:
1 - deal with unsafe operations inside unchecked blocks.
2 - remove unnecessary warplib functions and replace it for their respective operations.
3 - remove input checks.

given the following function:

function add(uint x, uint y) external pure returns (uint) {
    unchecked {
        return x + y;
    }
}

old transpilation:

#[view]
fn add_771602f7(__warp_0_x : u256, __warp_1_y : u256)-> u256{
    let __warp_se_0 = warp_add_unsafe256(__warp_0_x, __warp_1_y);
    return __warp_se_0;
}

new transpilation:

#[view]
fn add_771602f7(__warp_0_x : u256, __warp_1_y : u256)-> u256{
        let (__warp_se_0, overflow) = u256_overflowing_add(__warp_0_x, __warp_1_y);
        assert(overflow == false, 'Overflow in unchecked block');
        return __warp_se_0;
}

new transpilation for when the operation is bellow than 256 bits

#[view]
fn add_feb99390(__warp_0_x : u128, __warp_1_y : u128)-> u128{
        let (__warp_se_0, overflow) = matchu128_overflowing_add(__warp_0_x, __warp_1_y){
            Result::Ok(__warp_se_0) => (__warp_se_0, false),
            Result::Err(__warp_se_0) => (__warp_se_0, true),
        };
        assert(overflow == false, 'Overflow in unchecked block');
        return __warp_se_0;
}

In the case of an operation outside an unchecked block like the following:

function add(uint8 x, uint8 y)public returns(uint8){
        return x + y;
}

old transpilation:

------------imports---------------
use warplib::maths::add::warp_add8;
use warplib::maths::external_input_check_ints::warp_external_input_check_int8;

--------------------------------------

#[external]
fn add_bb4e3f4d(__warp_0_x : felt252, __warp_1_y : felt252)-> felt252{

    
    warp_external_input_check_int8(__warp_1_y);
    
    warp_external_input_check_int8(__warp_0_x);
    
    let __warp_se_0 = warp_add8(__warp_0_x, __warp_1_y);
    
    
    return __warp_se_0;
}

new transpilation:

------------No warplib imports-------------------------
 
#[external]
fn add_bb4e3f4d(__warp_0_x : felt252, __warp_1_y : felt252)-> felt252{
    
    return __warp_0_x + __warp_1_y;
}

@@ -41,33 +33,26 @@ export class MathsOperationToFunction extends ASTMapper {

visitBinaryOperation(node: BinaryOperation, ast: AST): void {
this.commonVisit(node, ast);
const isUnchecked = this.inUncheckedBlock;
if (
!((isUnchecked && (node.operator === '-' || node.operator === '+')) || node.operator === '**')
Copy link
Contributor

Choose a reason for hiding this comment

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

It's easier to understand non-negated conditions (especially when it's so complex). Could you modify the condition?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the whole pass can be removed. WYT @cicr99 ?

if (node.vInitialValue instanceof FunctionCall) {
if ((node.vInitialValue as FunctionCall).vExpression instanceof Identifier) {
const funcName = writer.write(node.vInitialValue);
if (funcName.includes('_overflow_') || funcName.includes('_overflowing_')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add TODO here and an issue to change it after its fixed in corelib

Comment on lines 36 to 37
if (node.vInitialValue instanceof FunctionCall) {
if ((node.vInitialValue as FunctionCall).vExpression instanceof Identifier) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (node.vInitialValue instanceof FunctionCall) {
if ((node.vInitialValue as FunctionCall).vExpression instanceof Identifier) {
if (node.vInitialValue instanceof FunctionCall && node.vInitialValue.vExpression instanceof Identifier) {

);
const width = getIntOrFixedByteBitWidth(retType);

const fullName = `u${width}_overflow${width === 256 && name === 'sub' ? '' : `ing`}_${name}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a TODO here too


const fullName = `u${width}_overflow${width === 256 && name === 'sub' ? '' : `ing`}_${name}`;

const importName = ['integer'];
Copy link
Contributor

@piwonskp piwonskp Jun 1, 2023

Choose a reason for hiding this comment

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

Let's make a function in importPaths.ts which returns the import path of overflowing functions based on the width. This way we can easily inspect cairo functions used in warp

@piwonskp
Copy link
Contributor

piwonskp commented Jun 1, 2023

Merge conflicts need to be solved

@piwonskp
Copy link
Contributor

piwonskp commented Jun 1, 2023

I've added a sibling issue #1079 to this one. It won't be using the old system of input checks though so I don't see any benefit in implementing it in the same PR

@esdras-santos esdras-santos force-pushed the math-operations-and-input-checks branch 2 times, most recently from 33f14f3 to 2aa4d95 Compare June 6, 2023 14:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants