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

Remove the copy-pasted reality.eth fork of reality.eth v3, replace it with reality.eth v4 via npm #250

Merged
merged 1 commit into from
Mar 7, 2024

Conversation

edmundedgar
Copy link
Contributor

No description provided.

@edmundedgar edmundedgar requested a review from josojo March 5, 2024 01:00
Copy link
Collaborator

@josojo josojo left a comment

Choose a reason for hiding this comment

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

Looks good. I just think that the balancerHOlder should not go into MinimalAdjudicationFramework, but into AdjudicationFrameworkREquests.
I think you can also pull the balanceHolder contract in from the NPM package and reuse the code, if you want and wanna save some lines

@@ -56,6 +61,8 @@ contract MinimalAdjudicationFramework is IMinimalAdjudicationFramework {
uint256 public forkActivationDelay;
IRealityETH public realityETH;

mapping(address => uint256) public balanceOf;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't get that change. Maybe you wanna add a comment why this is needed?

it seems the balanceOf() is never set to a positive value, can only be withdrawn. Strange

Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is only for the other contracts to inherit I think it should be put into the parent contract, or at least explained.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, you're right, it's only used in the pull version. I'll move the whole thing to there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I moved it to the pull version that allowed me to delete that stuff and just inherit the whole thing from the BalanceHolder in @reality.eth/contracts

@edmundedgar edmundedgar merged commit c8a9e32 into main Mar 7, 2024
3 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.

2 participants