Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

General repo improvements #109

Merged
merged 4 commits into from
Aug 24, 2023
Merged

General repo improvements #109

merged 4 commits into from
Aug 24, 2023

Conversation

vzotova
Copy link
Member

@vzotova vzotova commented Aug 7, 2023

  • fancier event check
  • solhint rules + hook
  • prettier action + hook

@vzotova vzotova changed the title [WIP] General repo improvements General repo improvements Aug 20, 2023
@vzotova vzotova marked this pull request as ready for review August 20, 2023 22:05
@vzotova vzotova requested review from KPrasch and theref August 20, 2023 22:05
.prettierignore Outdated Show resolved Hide resolved
.solhintignore Outdated Show resolved Hide resolved
assert event["startTimestamp"] == start
assert event["endTimestamp"] == end
assert event["cost"] == fee
assert tx.events == [
Copy link
Contributor

Choose a reason for hiding this comment

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

love this way of checking events 🚀

Copy link
Member

@derekpierre derekpierre left a comment

Choose a reason for hiding this comment

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

🎸

Comment on lines +96 to +97
_precomputed.pointEZyCoord
), // zE_y
Copy link
Member

@derekpierre derekpierre Aug 21, 2023

Choose a reason for hiding this comment

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

Minor suggestion where I disagree with the linter.

Suggested change
_precomputed.pointEZyCoord
), // zE_y
_precomputed.pointEZyCoord // zE_y
),

Some similar instances in later lines in this file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like there is open issue in prettier for that prettier/prettier#807
We can add hack for ignoring this line for prettier or just accept that. Wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

Not a huge issue to me. If the hack is easy great, if it's more involved, don't bother. Up to you really.

contracts/test/AdjudicatorTestSet.sol Outdated Show resolved Hide resolved
contracts/xchain/PolygonChild.sol Outdated Show resolved Hide resolved
assert event["stakingProvider"] == staking_provider
assert event["fromAmount"] == 0
assert event["toAmount"] == value
assert events == [
Copy link
Member

Choose a reason for hiding this comment

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

So much cleaner - 🚀

@vzotova vzotova requested a review from cygnusv August 21, 2023 19:17
uint256 constant PRECOMPUTED_DATA_SIZE = (20 * BIGNUM_SIZE) + 32 + 20 + 5;
uint256 internal constant BIGNUM_SIZE = 32;
uint256 internal constant POINT_SIZE = 33;
uint256 internal constant SIGNATURE_SIZE = 64;
Copy link

Choose a reason for hiding this comment

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

Yeah? Not DER encoded?

assert event["fromAmount"] == value // 2
assert event["toAmount"] == authorization
assert events == [
taco_application.AuthorizationInvoluntaryDecreased(
Copy link

Choose a reason for hiding this comment

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

badass. Love seeing events represented this way.

Copy link

@jMyles jMyles left a comment

Choose a reason for hiding this comment

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

As always, some of the nitty-gritty of solidity is a little tough for me to gork quickly, but I can verify that there's nothing here that looks crazy. :-)

@KPrasch KPrasch merged commit 3f9d0cc into nucypher:development Aug 24, 2023
2 checks passed
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.

5 participants