Skip to content

Function isOrderEncodable should also verify that the order's B value is not zero#333

Open
barakman wants to merge 2 commits intomainfrom
fix-isOrderEncodable
Open

Function isOrderEncodable should also verify that the order's B value is not zero#333
barakman wants to merge 2 commits intomainfrom
fix-isOrderEncodable

Conversation

@barakman
Copy link
Copy Markdown
Collaborator

@barakman barakman commented Dec 16, 2025

Note

Updates isOrderEncodable to return true only if encodeOrder(order).B > 0n, not merely if encoding succeeds.

Written by Cursor Bugbot for commit 6b97cc6. This will update automatically on new commits. Configure here.

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

This PR is being reviewed by Cursor Bugbot

Details

Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.

To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

try {
encodeOrder(order);
return true;
return encodeOrder(order).B > 0n;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe it is more correct to enforce this inside encodeOrder - and not in isOrderEncodable which is a function that seems to only be used in the tests

Copy link
Copy Markdown
Collaborator Author

@barakman barakman Apr 6, 2026

Choose a reason for hiding this comment

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

In the SDK of the new carbon version, I have ended up implementing it in a separate function named validOrder.

Other than getting rid of the "toxic" usage of try/catch (can fail for any other unrelated bug or error), it also seems the more correct way to handle order-creation.

In this version of carbon, the conditions are much more simple, and if you decide to adopt the suggestion above, then you'll just need to put your current checks in that function, and then add this check (B > 0) on top.

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.

2 participants