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.

Sign up for GitHub

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

Jump to bottom

Implement ACP-176 math #783

Merged
merged 8 commits into from
Feb 11, 2025
Merged

Implement ACP-176 math #783

merged 8 commits into from
Feb 11, 2025

Conversation

StephenButtolph
Copy link
Contributor

Why this should be merged

This PR implements all of the formulas specified by ACP-176.

How this works

Adds the implementation and extensive tests.

How this was tested

  1. Unit tests
  2. Utilized this in a local network to handle fees.

Need to be documented?

No.

Need to update RELEASES.md?

No.

@StephenButtolph StephenButtolph requested review from ceyonur, darioush and a team as code owners February 7, 2025 22:03
darioush
darioush previously approved these changes Feb 7, 2025
View reviewed changes
plugin/evm/acp176/acp176.go Outdated Show resolved Hide resolved
darioush
darioush previously approved these changes Feb 8, 2025
View reviewed changes
michaelkaplan13
michaelkaplan13 previously approved these changes Feb 9, 2025
View reviewed changes
Copy link

@michaelkaplan13 michaelkaplan13 left a comment

Choose a reason for hiding this comment

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

One question on naming and one question on rounding, but otherwise LGTM.

qdm12
qdm12 reviewed Feb 10, 2025
View reviewed changes
plugin/evm/acp176/acp176.go Outdated Show resolved Hide resolved
plugin/evm/acp176/acp176.go Outdated Show resolved Hide resolved
plugin/evm/acp176/acp176_test.go Show resolved Hide resolved
plugin/evm/acp176/acp176_test.go Outdated Show resolved Hide resolved
plugin/evm/acp176/acp176.go Show resolved Hide resolved
plugin/evm/acp176/acp176_test.go Show resolved Hide resolved
ARR4N
ARR4N reviewed Feb 10, 2025
View reviewed changes
Copy link
Contributor

@ARR4N ARR4N left a comment

Choose a reason for hiding this comment

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

First pass at readability of the implementation, not tests. I saw that you tagged me and will address that question separately.

plugin/evm/acp176/acp176.go Outdated Show resolved Hide resolved
plugin/evm/acp176/acp176.go Outdated Show resolved Hide resolved
plugin/evm/acp176/acp176.go Outdated Show resolved Hide resolved
plugin/evm/acp176/acp176.go Outdated Show resolved Hide resolved
plugin/evm/acp176/acp176.go Outdated Show resolved Hide resolved
plugin/evm/acp176/acp176.go Outdated Show resolved Hide resolved
plugin/evm/acp176/acp176.go Outdated Show resolved Hide resolved
plugin/evm/acp176/acp176.go Outdated Show resolved Hide resolved
plugin/evm/acp176/acp176.go Outdated Show resolved Hide resolved
plugin/evm/acp176/acp176.go Outdated Show resolved Hide resolved
ARR4N
ARR4N approved these changes Feb 11, 2025
View reviewed changes
Copy link
Contributor

@ARR4N ARR4N left a comment

Choose a reason for hiding this comment

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

LGTM for readability (with a cursory nod to a review of correctness too, but there are many other eyes on this).

@StephenButtolph StephenButtolph merged commit 986b18e into master Feb 11, 2025
8 checks passed
@StephenButtolph StephenButtolph deleted the acp-176-implement-math branch February 11, 2025 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qdm12 qdm12 qdm12 left review comments

@alarso16 alarso16 alarso16 left review comments

@ARR4N ARR4N ARR4N approved these changes

@darioush darioush darioush approved these changes

@michaelkaplan13 michaelkaplan13 michaelkaplan13 left review comments

@ceyonur ceyonur Awaiting requested review from ceyonur ceyonur is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@StephenButtolph @ARR4N @darioush @qdm12 @michaelkaplan13 @alarso16