feat(payments): implement order abis

[MohammadNassar1]

This change is 

[ishay-starkware]

src/payments.cairo line 101 at r1 (raw file):

        fn remove_token(ref self: ContractState, token: ContractAddress) {}

        fn cancel_orders(ref self: ContractState, orders: Span<Order>) {

i believe that same as in trade - only the Operator can submit trades I so think that also here only the Operator can cancel_orders so the check needs to be only_operator and not that the caller == order.address

[MohammadNassar1]

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @ishay-starkware)

---

src/payments.cairo line 101 at r1 (raw file):

Previously, ishay-starkware wrote…

i believe that same as in trade - only the Operator can submit trades I so think that also here only the Operator can cancel_orders so the check needs to be only_operator and not that the caller == order.address

Done.

[ishay-starkware]

Reviewed 2 of 3 files at r1.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @MohammadNassar1)

---

src/errors.cairo line 3 at r2 (raw file):

pub const INVALID_ZERO_ADDRESS: felt252 = 'INVALID_ZERO_ADDRESS';
pub const TOKEN_ALREADY_REGISTERED: felt252 = 'TOKEN_ALREADY_REGISTERED';
pub const TOKEN_NOT_REGISTERED: felt252 = 'TOKEN_NOT_REGISTERED';

not part of this pr

Code quote:

pub const INVALID_ZERO_ADDRESS: felt252 = 'INVALID_ZERO_ADDRESS';
pub const TOKEN_ALREADY_REGISTERED: felt252 = 'TOKEN_ALREADY_REGISTERED';
pub const TOKEN_NOT_REGISTERED: felt252 = 'TOKEN_NOT_REGISTERED';

[MohammadNassar1]

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @ishay-starkware and @MohammadNassar1)

---

src/errors.cairo line 3 at r2 (raw file):

Previously, ishay-starkware wrote…

not part of this pr

These were added in the previous PR.
Note that no change in this file.

[ishay-starkware]

Reviewed 2 of 3 files at r2.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @MohammadNassar1)

---

src/payments.cairo line 124 at r2 (raw file):

                // TODO(Mohammad): Replace with actual hash computation logic.
                let order_hash: HashType = Default::default();
                self.fulfillment.write(order_hash, order.amount_a.abs());

change the value of the map to an Enum that has variants of Canceled, Fulfilled, Partial_Fulfilled, Not_Exist (default) and that can save amounts that was already fulfilled from the order

[MohammadNassar1]

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @ishay-starkware and @MohammadNassar1)

---

src/payments.cairo line 124 at r2 (raw file):

Previously, ishay-starkware wrote…

change the value of the map to an Enum that has variants of Canceled, Fulfilled, Partial_Fulfilled, Not_Exist (default) and that can save amounts that was already fulfilled from the order

Done.

[ishay-starkware]

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @MohammadNassar1)

---

src/interface.cairo line 9 at r3 (raw file):

    PartialFulfilled: u128,
    Canceled: u128,
    Fulfilled,

i think we better keep this with the amount as well

[ishay-starkware]

Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MohammadNassar1)

[MohammadNassar1]

Reviewable status: 2 of 4 files reviewed, 1 unresolved discussion (waiting on @ishay-starkware)

---

src/interface.cairo line 9 at r3 (raw file):

Previously, ishay-starkware wrote…

i think we better keep this with the amount as well

Done.

[RoeeGross]

src/interface.cairo line 5 at r4 (raw file):

#[derive(Copy, Drop, Debug, Serde, PartialEq, starknet::Store)]
pub enum FulfilledStatus {

we need to check if this enum size is only one felt

[RoeeGross]

Reviewed all commit messages.
Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @ishay-starkware and @MohammadNassar1)

[RoeeGross]

src/payments.cairo line 147 at r4 (raw file):

        }

        fn cancel_orders(ref self: ContractState, orders: Span<Order>) {

let's keep it hash

move the handle of sigle order to a private function

also consider to create version with single order

[MohammadNassar1]

Reviewable status: 2 of 5 files reviewed, 3 unresolved discussions (waiting on @ishay-starkware and @RoeeGross)

---

src/payments.cairo line 147 at r4 (raw file):

Previously, RoeeGross wrote…

let's keep it hash

move the handle of sigle order to a private function

also consider to create version with single order

Done.

[RoeeGross]

src/payments.cairo line 212 at r6 (raw file):

            match self.fulfillment.read(order_hash) {
                FulfilledStatus::Fulfilled(_) => { panic_with_felt252(ORDER_WAS_FULFILLED); },
                FulfilledStatus::PartialFulfilled(fulfilled_amount) => {

a point for the future we maybe need to cancel the approve also

[RoeeGross]

Reviewed 2 of 4 files at r6.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @ishay-starkware and @MohammadNassar1)

[MohammadNassar1]

Reviewable status: all files reviewed (commit messages unreviewed), 9 unresolved discussions (waiting on @ishay-starkware and @RoeeGross)

---

src/interface.cairo line 10 at r6 (raw file):

Previously, RoeeGross wrote…

why do we need u128 in case of Canceld?
what is mean?
please add some docs
(As I understand now, the u128 means the consume amount and in the case of Fulfilled
you can ignore it

It shows the amount that was fulfilled before the order was canceled.

---

src/payments.cairo line 212 at r6 (raw file):

Previously, RoeeGross wrote…

a point for the future we maybe need to cancel the approve also

Now the approval is not part of the contract.

---

src/tests/test_payments.cairo line 151 at r6 (raw file):

Previously, RoeeGross wrote…

array

wdym? :)
The hash is felt252.

---

src/tests/test_payments.cairo line 155 at r6 (raw file):

Previously, RoeeGross wrote…

loop and also check the order_hash_3

Done.

---

src/tests/test_payments.cairo line 168 at r6 (raw file):

Previously, RoeeGross wrote…

you can also need to chech the case of cancel partial full...
you can do that by edit the storage

Correct, but partial fulfillment requires a flow test that involves trade. It will be done once the trade is merged.

---

src/tests/test_payments.cairo line 171 at r6 (raw file):

Previously, RoeeGross wrote…

loop

Done.

---

src/tests/test_payments.cairo line 184 at r6 (raw file):

Previously, RoeeGross wrote…

add before this line chect call adrees with the adrres 'non operator'
so the test will be more explicity

Done.

[RoeeGross]

src/interface.cairo line 10 at r6 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

It shows the amount that was fulfilled before the order was canceled.

Okay, so let's remove the u128 from Fulfilled
or remove the Fulfilled variant

[RoeeGross]

src/payments.cairo line 212 at r6 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Now the approval is not part of the contract.

But we need to clear the approval.

[RoeeGross]

src/tests/test_payments.cairo line 151 at r6 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

wdym? :)
The hash is felt252.

create array of felt252 and instead of
oreder_hash_i
you got
order_hash[i]

[RoeeGross]

src/tests/test_payments.cairo line 168 at r6 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Correct, but partial fulfillment requires a flow test that involves trade. It will be done once the trade is merged.

you can test it without the trade flow
just wdit the storage and set it to be cancel

[RoeeGross]

Reviewed 1 of 2 files at r8.
Reviewable status: 4 of 5 files reviewed, 6 unresolved discussions (waiting on @ishay-starkware and @MohammadNassar1)

[MohammadNassar1]

Reviewable status: 4 of 5 files reviewed, 6 unresolved discussions (waiting on @ishay-starkware and @RoeeGross)

---

src/interface.cairo line 10 at r6 (raw file):

Previously, RoeeGross wrote…

Okay, so let's remove the u128 from Fulfilled
or remove the Fulfilled variant

@ishay-starkware requested here to keep the u128 for fulfilled.

---

src/payments.cairo line 212 at r6 (raw file):

Previously, RoeeGross wrote…

But we need to clear the approval.

Hmm, why should this be part of the contract?
Since approval itself isn’t part of the contract, I’d expect the approval clearing to also be handled outside of it.

wdyt?

---

src/tests/test_payments.cairo line 151 at r6 (raw file):

Previously, RoeeGross wrote…

create array of felt252 and instead of
oreder_hash_i
you got
order_hash[i]

Done.

---

src/tests/test_payments.cairo line 168 at r6 (raw file):

Previously, RoeeGross wrote…

you can test it without the trade flow
just wdit the storage and set it to be cancel

Done.

[RoeeGross]

src/payments.cairo line 212 at r6 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Hmm, why should this be part of the contract?
Since approval itself isn’t part of the contract, I’d expect the approval clearing to also be handled outside of it.

wdyt?

It's not part of the contract but we cancel some order so it's make sense to clear every 'resources'
but I suggest to ask @remollemo

[RoeeGross]

src/tests/test_payments.cairo line 151 at r6 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Done.

not done....

[RoeeGross]

Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ishay-starkware, @MohammadNassar1, and @remollemo)

[MohammadNassar1]

Reviewable status: 4 of 5 files reviewed, 4 unresolved discussions (waiting on @ishay-starkware, @remollemo, and @RoeeGross)

---

src/interface.cairo line 5 at r4 (raw file):

Previously, RoeeGross wrote…

we need to check if this enum size is only one felt

I see that the enum is using 2 felts.
I see it when using store/load, it returns an array of 2 felts. one felt for the enum, and other for the amount.

---

src/payments.cairo line 212 at r6 (raw file):

Previously, RoeeGross wrote…

It's not part of the contract but we cancel some order so it's make sense to clear every 'resources'
but I suggest to ask @remollemo

Will discuss it with @remollemo.

---

src/tests/test_payments.cairo line 151 at r6 (raw file):

Previously, RoeeGross wrote…

not done....

Changed to an array,
Please note that at method returns reference, so I had to add deref when using the values.

[RoeeGross]

src/tests/test_payments.cairo line 151 at r6 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Changed to an array,
Please note that at method returns reference, so I had to add deref when using the values.

#[test]
fn test_successful_handle_order() {
let contract_address = init_contract_with_roles();
let dispatcher = IPaymentsDispatcher { contract_address };
let order_hashes = array!['order_hash_1', 'order_hash_2', 'order_hash_3'];

for order_hash in order_hashes.span() {
    assert_eq!(
        dispatcher.get_order_fulfillment(*order_hash), FulfilledStatus::PartialFulfilled(0),
    );
}

cheat_caller_address_once(:contract_address, caller_address: testing_constants::OPERATOR);
dispatcher.cancel_orders(order_hashes: array![*(order_hashes[0])].span());
assert_eq!(dispatcher.get_order_fulfillment(*(order_hashes[0])), FulfilledStatus::Canceled(0));
for order_hash in order_hashes.span().slice(1, 2) {
    assert_eq!(
        dispatcher.get_order_fulfillment(*order_hash), FulfilledStatus::PartialFulfilled(0),
    );
}

[RoeeGross]

Reviewable status: 4 of 5 files reviewed, 4 unresolved discussions (waiting on @ishay-starkware, @MohammadNassar1, and @remollemo)

---

src/interface.cairo line 5 at r4 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

I see that the enum is using 2 felts.
I see it when using store/load, it returns an array of 2 felts. one felt for the enum, and other for the amount.

so, we need to add store packing

[MohammadNassar1]

Reviewable status: 4 of 5 files reviewed, 4 unresolved discussions (waiting on @ishay-starkware, @remollemo, and @RoeeGross)

---

src/tests/test_payments.cairo line 151 at r6 (raw file):

Previously, RoeeGross wrote…

#[test]
fn test_successful_handle_order() {
let contract_address = init_contract_with_roles();
let dispatcher = IPaymentsDispatcher { contract_address };
let order_hashes = array!['order_hash_1', 'order_hash_2', 'order_hash_3'];

for order_hash in order_hashes.span() {
    assert_eq!(
        dispatcher.get_order_fulfillment(*order_hash), FulfilledStatus::PartialFulfilled(0),
    );
}

cheat_caller_address_once(:contract_address, caller_address: testing_constants::OPERATOR);
dispatcher.cancel_orders(order_hashes: array![*(order_hashes[0])].span());
assert_eq!(dispatcher.get_order_fulfillment(*(order_hashes[0])), FulfilledStatus::Canceled(0));
for order_hash in order_hashes.span().slice(1, 2) {
    assert_eq!(
        dispatcher.get_order_fulfillment(*order_hash), FulfilledStatus::PartialFulfilled(0),
    );
}

Done.

[RoeeGross]

Reviewed 1 of 1 files at r11.
Reviewable status: all files reviewed (commit messages unreviewed), 2 unresolved discussions (waiting on @ishay-starkware and @MohammadNassar1)

[RoeeGross]

src/interface.cairo line 5 at r4 (raw file):

Previously, RoeeGross wrote…

so, we need to add store packing

Please use store packing for each "starknet::Store" if you can

[RoeeGross]

Reviewed 1 of 4 files at r3, 2 of 4 files at r6, 1 of 1 files at r11, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ishay-starkware and @MohammadNassar1)

[RoeeGross]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ishay-starkware)

[RoeeGross]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ishay-starkware)

Gross has approved the PR.

[MohammadNassar1]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ishay-starkware)

---

src/interface.cairo line 5 at r4 (raw file):

Previously, RoeeGross wrote…

Please use store packing for each "starknet::Store" if you can

Let's do it in a separate PR.