Skip to content

Conversation

@MohammadNassar1
Copy link
Collaborator

@MohammadNassar1 MohammadNassar1 commented Jul 23, 2025

This change is Reviewable

@MohammadNassar1 MohammadNassar1 self-assigned this Jul 23, 2025
@MohammadNassar1 MohammadNassar1 force-pushed the mohammad/payments/add-payments-interface branch 2 times, most recently from a2dd43e to 380911b Compare July 23, 2025 15:38
@MohammadNassar1 MohammadNassar1 force-pushed the mohammad/payments/impl-setters-and-getters branch 2 times, most recently from 27b5608 to 0d5819b Compare July 23, 2025 15:59
@ishay-starkware ishay-starkware deleted the branch main August 3, 2025 08:34
@MohammadNassar1 MohammadNassar1 changed the base branch from mohammad/payments/add-payments-interface to main August 3, 2025 08:51
@MohammadNassar1 MohammadNassar1 force-pushed the mohammad/payments/impl-setters-and-getters branch from 0d5819b to 2e796d7 Compare August 3, 2025 09:58
Copy link
Collaborator

@ishay-starkware ishay-starkware left a comment

Choose a reason for hiding this comment

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

you can also separate each getter and setter to a different PR.
also - no tests

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


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

                self.fulfillment.write(order_hash, order.amount_a.abs());
            }
        }

separate this to a different PR - this pr is only for getters and setters as the title suggest.
you can also add the Order struct in a different PR and then you won't need the TODO

Code quote:

        fn cancel_orders(ref self: ContractState, orders: Span<Order>) {
            let caller = get_caller_address();

            for order in orders {
                assert(*order.address == caller, INVALID_CALLER_ADDRESS);

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

@ishay-starkware
Copy link
Collaborator

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

            let ordered_amount = order.amount_a.abs();
            ordered_amount == fulfilled_amount
        }

also in a different pr after you have the hash.

Code quote:

        fn is_order_fulfilled(self: @ContractState, order: Order) -> bool {
            // TODO(Mohammad): Replace with actual hash computation logic.
            let order_hash: HashType = Default::default();
            let fulfilled_amount = self.fulfillment.read(order_hash);
            let ordered_amount = order.amount_a.abs();
            ordered_amount == fulfilled_amount
        }

@ishay-starkware
Copy link
Collaborator

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

            assert(fee <= MAX_BASIS_POINTS.into(), INVALID_HIGH_FEE);
            self.fee.write(fee);

do this in a private fn called _set_fee and then call it from the constructor as well

Code quote:

            assert(fee <= MAX_BASIS_POINTS.into(), INVALID_HIGH_FEE);
            self.fee.write(fee);

@ishay-starkware
Copy link
Collaborator

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

            assert(recipient.is_non_zero(), INVALID_ZERO_ADDRESS);
            self.fee_recipient.write(recipient);

same

Code quote:

            assert(recipient.is_non_zero(), INVALID_ZERO_ADDRESS);
            self.fee_recipient.write(recipient);

@ishay-starkware
Copy link
Collaborator

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

        ) {}

        fn register_token(ref self: ContractState, token: ContractAddress) {

you should probably have a is_token_registered fn as well

@ishay-starkware
Copy link
Collaborator

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

            assert(token.is_non_zero(), INVALID_ZERO_ADDRESS);
            assert(!self.tokens.read(token), TOKEN_ALREADY_REGISTERED);

here call the is_token_registered

@ishay-starkware
Copy link
Collaborator

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

        ) {}

        fn register_token(ref self: ContractState, token: ContractAddress) {

register_token and remove_token should be in a separate PR

Copy link
Collaborator Author

@MohammadNassar1 MohammadNassar1 left a comment

Choose a reason for hiding this comment

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

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


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

Previously, ishay-starkware wrote…

you should probably have a is_token_registered fn as well

Done.
here


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

Previously, ishay-starkware wrote…

register_token and remove_token should be in a separate PR

Done.
here


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

Previously, ishay-starkware wrote…

here call the is_token_registered

Done.
here

@MohammadNassar1 MohammadNassar1 force-pushed the mohammad/payments/impl-setters-and-getters branch from 2e796d7 to f46b7bd Compare August 3, 2025 12:05
Copy link
Collaborator Author

@MohammadNassar1 MohammadNassar1 left a comment

Choose a reason for hiding this comment

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

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


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

Previously, ishay-starkware wrote…

separate this to a different PR - this pr is only for getters and setters as the title suggest.
you can also add the Order struct in a different PR and then you won't need the TODO

Moved to another PR.
I'll keep it for now with the todo, as I got stuck with the order hash.


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

Previously, ishay-starkware wrote…

do this in a private fn called _set_fee and then call it from the constructor as well

Done.


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

Previously, ishay-starkware wrote…

same

Done.


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

Previously, ishay-starkware wrote…

also in a different pr after you have the hash.

Moved to another PR.
I'll keep it for now with the todo, as I got stuck with the order hash.

@MohammadNassar1 MohammadNassar1 force-pushed the mohammad/payments/impl-setters-and-getters branch from f46b7bd to c6cff51 Compare August 3, 2025 12:09
Copy link
Collaborator Author

@MohammadNassar1 MohammadNassar1 left a comment

Choose a reason for hiding this comment

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

done

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

@ishay-starkware
Copy link
Collaborator

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

    pub impl ImplInternalPayments of InternalPaymentsTrait {
        fn _set_fee(ref self: ContractState, fee: u128) {
            assert(fee <= MAX_BASIS_POINTS.into(), INVALID_HIGH_FEE);

i assume there should be some max fee as setting the fee to MAX_BASIS_POINTS doesn't make much sense

Copy link
Collaborator

@ishay-starkware ishay-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 4 files at r2, all commit messages.
Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @MohammadNassar1)


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

pub const INVALID_CALLER_ADDRESS: felt252 = 'INVALID_CALLER_ADDRESS';

remove

Code quote:

ub const INVALID_CALLER_ADDRESS: felt252 = 'INVALID_CALLER_ADDRESS';

@MohammadNassar1 MohammadNassar1 force-pushed the mohammad/payments/impl-setters-and-getters branch from c6cff51 to 9a02957 Compare August 3, 2025 14:39
Copy link
Collaborator Author

@MohammadNassar1 MohammadNassar1 left a comment

Choose a reason for hiding this comment

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

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


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

Previously, ishay-starkware wrote…

remove

Done.


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

Previously, ishay-starkware wrote…

i assume there should be some max fee as setting the fee to MAX_BASIS_POINTS doesn't make much sense

Done.

@MohammadNassar1 MohammadNassar1 force-pushed the mohammad/payments/impl-setters-and-getters branch from 9a02957 to 91ec3bb Compare August 3, 2025 15:46
@MohammadNassar1 MohammadNassar1 force-pushed the mohammad/payments/impl-setters-and-getters branch from 91ec3bb to 2360859 Compare August 4, 2025 08:35
Copy link
Collaborator

@ishay-starkware ishay-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @MohammadNassar1)

@ishay-starkware ishay-starkware merged commit 96de594 into main Aug 4, 2025
3 checks passed
@ishay-starkware ishay-starkware deleted the mohammad/payments/impl-setters-and-getters branch August 4, 2025 12:59
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