Skip to content

General repo improvements#109

Merged
KPrasch merged 4 commits intonucypher:developmentfrom
vzotova:impr
Aug 24, 2023
Merged

General repo improvements#109
KPrasch merged 4 commits intonucypher:developmentfrom
vzotova:impr

Conversation

@vzotova
Copy link
Copy Markdown
Member

@vzotova vzotova commented Aug 7, 2023

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

@vzotova vzotova self-assigned this Aug 7, 2023
@vzotova vzotova force-pushed the impr branch 20 times, most recently from eef8522 to 2a57578 Compare August 14, 2023 20:57
@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
assert event["startTimestamp"] == start
assert event["endTimestamp"] == end
assert event["cost"] == fee
assert tx.events == [
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.

love this way of checking events 🚀

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

assert event["stakingProvider"] == staking_provider
assert event["fromAmount"] == 0
assert event["toAmount"] == value
assert events == [
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown

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
Copy Markdown

@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
@vzotova vzotova deleted the impr branch January 28, 2025 18:19
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