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

[Feature] Billing Contract Integration #127

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

nup9151f
Copy link
Contributor

Summary

This PR adds a Billing contract that allows collector to create bills for different apps and users to pay those bills.

Description

Billing contract

Current implementation is based on edgeandnode/Billing contract adapted to our case and discussion on OF-49 and OF-54.

This PR does not touch current platform fee mechanism.

Changes:

  • Added Billing contract
  • Added Governed and Rescuable extensions for Billing contract
  • Added BillingChecker extension that can be used by facets to check whether the app has paid its bills
  • Added functions to Globals contract to get/set Billing contract for all apps
  • Added environments vars that are used to create the Billing contract when deploying it.
  • Added deploy script to deploy Billing contract and register its address in Globals contract
    -Added Billing contract to deployment process in Makefile

In order to deploy Billing contract we need to specify several environment variables:

  • GOVERNOR_ADDRESS - Address of user that will function as contract governor.
  • COLLECTOR_ADDRESS - Address of user that will function as initial collector, more collector can be added later.
  • OFT_TOKEN_ADDRESS - Address of the already deployer ERC20 token that will be used as currency.

Related Issue/Bounty

Ref: OF-116

Type of Change

Please check the boxes that apply to your pull request:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update (changes to documentation only)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes)
  • Other (please describe):

Testing

Contract functionalities and integration are tested in

  • Test test/billing/Billing.t.sol for basic functionalities
  • Test test/integration/billing/BillingChecker.sol for hasPaid integration

Checklist

  • I have read the CONTRIBUTING.md document
  • I have followed the project's style guide
  • I have added or updated tests to cover my changes
  • All new and existing tests passed
  • My changes generate no new warnings or errors
  • I have made corresponding changes to the documentation
  • I have checked that my changes do not introduce any new security vulnerabilities
  • I have provided a clear and concise description of my changes
  • I have linked to the related issue or bounty, if applicable

@nup9151f nup9151f added the enhancement New feature or request label May 20, 2024
@nup9151f nup9151f self-assigned this May 20, 2024
Copy link

changeset-bot bot commented May 20, 2024

⚠️ No Changeset found

Latest commit: 20acf66

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

@george-openformat george-openformat left a comment

Choose a reason for hiding this comment

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

Incredible! This is fantastic.

I've spotted a potential roadblock moving forward related to how the globals contract was deployed. I'm gonna create another issue to tackle that. For now if we can keep a copy of the globals.sol contract was without changes i'd be happy to merge.

Some potential moving of files as well happy to discuss

Copy link
Contributor

Choose a reason for hiding this comment

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

// TODO: deploy behind a upgradeable proxy see solidstate-solidity/contracts/proxy/upgradeable/UpgradeableProxyOwnable.sol

Sorry for not noticing this before. We might run into an issue here as globals, from what I can see, was not deployed behind an upgradable proxy. This means we can't upgrade it and if we deploy a new one will mean we have to manually set globals on the apps some how.

Here is my current thinking:

There is another approach we can take by hardcoding the address into the BillingCheckerInternal.sol but this is going to cause problems with having to manually update that address when testing and deploying which is bound to go wrong at some point. So I don't think it is worth pursuing further.

// SPDX-License-Identifier: BUSL-1.1
pragma solidity ^0.8.16;

import {Global} from "../global/Global.sol";
import {IBillingChecker} from "./IBillingChecker.sol";
import {IBilling} from "../../billing/IBilling.sol";

// hardcoded billing conrtact address
address constant billingContract = 0x123...;

abstract contract BillingCheckerInternal is IBillingChecker, Global {
    /**
     * @dev wrapper that calls hasPaid from billing contract configured in Globals contract
     */
    function _hasPaid(address _app) internal view returns (bool) {
        if ( billingContract == address(0) ) {
            return false;
        }
        return IBilling(billingContract).hasPaid(_app);
    }
}

I think what you have is the best approach but we need to find a way to redeploy Globals.sol behind an upgradable proxy and migrate all existing apps over. In theory we could have a facet that gives our open formats wallet privileges to set the global address on all apps. And we create a script to upload that facet. update the globals address on all apps then remove the facet.

For now can we add an OldGlobals.sol contract without these changes, so we can write up and test these migration scripts later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering because these don't follow the diamond storage pattern, should we move this into billing/governed/Governed.sol

The reason being is that if a contract inheriting this uses the diamond storage pattern it will likely break the upgradability because of how variables are stored.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same here should we move this to src/billing/rescuable/Rescuable.sol ?

@tinypell3ts tinypell3ts removed the request for review from refugene July 8, 2024 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants