New syntax for contract method calls

[mohammadfawaz]

Closes #298

Syntax

Old syntax:

abi Wallet {
    fn send_funds(gas: u64, coins_to_forward: u64, asset_id: b256, req: SendFundsRequest);
}

impl Wallet for Contract {
     fn send_funds(gas_to_forward: u64, coins_to_forward: u64, asset_id: b256, req: SendFundsRequest) {
        ...
    }
}
...
fn main() {
    let contract_address = 0x9299da6c73e6dc03eeabcce242bb347de3f5f56cd1c70926d76526d7ed199b8b;
    let caller = abi(Wallet, contract_address);
    let req = SendFundsRequest {
        amount_to_send: 200,
        recipient_address: 0x9299da6c73e6dc03eeabcce242bb347de3f5f56cd1c70926d76526d7ed199b8b,
    };
    caller.send_funds(10000, 0, ETH_ID, req);
}

New syntax:

abi Wallet {
    fn send_funds(amount_to_send: u64, recipient_address: b256);
}

impl Wallet for Contract {
     fn send_funds(amount_to_send: u64, recipient_address: b256) {
        ...
    }
}
...
fn main() {
    let contract_address = 0x9299da6c73e6dc03eeabcce242bb347de3f5f56cd1c70926d76526d7ed199b8b;
    let caller = abi(Wallet, contract_address);
    
    caller.send_funds{
        gas: 10000, // Can be skipped
        coins: 0, // Can be skipped or set to something other than the default value
        asset_id: ETH_ID // Can be skipped or set to something other than the default value
    } 
    (200, 0x9299da6c73e6dc03eeabcce242bb347de3f5f56cd1c70926d76526d7ed199b8b);
}

• There is no need to wrap the user arguments into a struct anymore and any number of arguments can be used.
• There is no need to pass () when no arguments are required.
• The contract parameters gas, coins, and asset_id are optional and they can be specified in any order. If a parameter is skipped, its default value is used in the CALL instruction. The default values for gas, coins, and asset_id are $cgas, 0, and ETH_ID respectively. ETH_ID is stored as a compiler constant.
• When none of the call parameters are set, both of these syntaxes work: caller.send_funs{}(...) or caller.send_funds().
• Specifying any of the parameters more than once is a compile error.
• Specifying a parameter other than gas, coins, or asset_id is a compile error.

Implementation

The implementation is simple:

• On the caller side, gather the contract parameters, assign them to registers, and use them in the CALL instruction as before. Then, create a struct of all user arguments and follow the same flow that is used for user-defined struct.
• On the contract side, create a struct type that contains the user arguments and re-use some of the code used for subfields to handle field offsets and so on.
• On both sides, no wrapper struct is created if only a single argument is passed.

Testing

All contract tests were failing before this change because the hardcoded contract IDs in the calling scripts were outdated (+ some other minor issues). I updated all the required contract IDs here but there are 2 tests that continue to fail (even on master), namely:

• ("context_testing_contract", "caller_context_test")
• ("contract_abi_impl", "contract_call")

Editi. Updated the default values for the parameters.

Generated assembly on the caller side in the default case:

lw   $r1 data_7               ; load fn selector for call
lw   $r5 $cgas i0             ; loading $cgas (gas) into abi function
lw   $r4 data_4               ; loading the default coins value for call
lw   $r3 data_5               ; loading the default asset_id value for call
...
data_4 .u64 0x00
data_5 .b256 0x0000000000000000000000000000000000000000000000000000000000000000
call $r0 $r4 $r3 $r5

Generated assembly on the caller side in the non-default case:

lw   $r5 data_3               ; literal instantiation
lw   $r4 data_8               ; literal instantiation
lw   $r3 data_9               ; literal instantiation
...
call $r0 $r4 $r3 $r5          ; call external contract
...
data_3 .u64 0x2710
data_8 .u64 0x32
data_9 .b256 0xfffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff0

[mohammadfawaz]

@adlerjohn @nfurfaro FYI - You can check out the new syntax in action in the updated tests :)

[otrho]

Cool! I noticed in the tests it's generally all or nothing with the contract args? If e.g., coins was zero can you just leave it out of that map thing?

[mohammadfawaz]

Cool! I noticed in the tests it's generally all or nothing with the contract args? If e.g., coins was zero can you just leave it out of that map thing?

Actually, I'd like someone to confirm that what I did was correct in the default case. I assumed that the default values (that #298 talks about) are basically the contents of $cgas, $bal, and $fp for gas, coins, and asset_id respectively. Is this correct? Or are there some default constants that I should be aware of?

[adlerjohn]

The default value for a parameter is extracted from the appropriate special register ($cgas for gas, $bal for coins, and $fp for asset_id)

Wait no, those are more for receiving than sending. The defaults should be $cgas, 0, and ETH_ID, in order.

[adlerjohn]

LGTM, but will defer to @otrho and @sezna

[nfurfaro]

I think the new syntax looks great @mohammadfawaz !
As for the failing test ("context_testing_contract", "caller_context_test"), I'm working to port the context tests to the stdlib repo using the SDK anyway, so will remove the redundant ones here when I'm done.

[adlerjohn]

Wait, actually.

Instead of using tag, can you use branch = "master" and a Forc lockfile?

[mohammadfawaz]

Wait, actually.

Instead of using tag, can you use branch = "master" and a Forc lockfile?

#876 should bulk change all the tests right? I'm not really adding any new tests here.

[adlerjohn]

Oh right it's too early in the morning and I forget that wasn't actually changed yet

[adlerjohn]

Could you update the PR description on how the new ABI syntax will look? Currently it still has the 4 parameters.

[mohammadfawaz]

Could you update the PR description on how the new ABI syntax will look? Currently it still has the 4 parameters.

Ha! Good catch. Fixed!

[sezna]

LGTM -- left one comment

[sezna]

BTW, I think loading $zero would also be valid here as 0 is shorthand for forwarding all remaining gas. CC @adlerjohn for a correctness check on that

[mohammadfawaz]

I merged for now while I can. Will follow up with John on Monday and change if necessary :)