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

Re-structuring and renaming of contracts code. #71

Merged
merged 86 commits into from
Jul 5, 2024

Conversation

jplodge-pro
Copy link
Member

@jplodge-pro jplodge-pro commented Jul 2, 2024

NOTE: This PR is #66 ff-merged to give a cleaner change profile.

Reason

This is a quick change to address some of the issues I have with the contracts codebase, like:

  1. Naming, for example:
    • CredbullBaseVault the root of the Vault contract hierarchy.
      • Prefixed with Credbull, but not actually deployed (the only other 'Credbull' prefixed contracts are those that are deployed).
      • Base, but is that really necessary? Also, in that vein, it could be BaseMaturityVault.
    • We have MockToken and MockStableCoin in test, but also CredbullBaseVaultMock (same for all vaults).
  2. Inconsistent naming (contracts, structs, files), for example:
    • whitelist and kyc used interchangeably.
    • For WindowPlugIn is it a mature window, or a redemption window? Seeing as the other window is the deposit window, it seems to me that it should be redemption. But it's IS mature and is referenced interchangeably as 'mature' and 'redemption'.
  3. Uncategorised (as far as I can tell) directory structure. For example:
    • base, vaults AND extensions fulfilling essentially the same purpose.
    • interface containing cross-functional interfaces.
      • IErrors has been deleted
    • Root (src) containing deployable contracts AND factories

Adopted Changes

  • Categorised singular sub-directories, like vault and factory.
    • image
    • image
  • All deployable contracts are in the root of the src directory.
    • image
  • All things PlugIn are now re-named Plugin.
  • All Vault/Plugin Contracts have a XParams struct capturing their construction values.
    • NOTE: When an XParams contains a reference to another XParams, the member name is x.
      • image
    • This will lead to a usage pattern like this: params.maturityVault.vault.custodian;, where params is a FixedYieldVaultParams. Thus the purpose is clear due to the call chain off params and no need to have an xParams at every level of the tree.
  • All Vault Contract constructors accept an instance of their parameters struct.
  • Changed the matureWindow in WindowPlugIn to redemptionWindow.
    • As per the discussion on #engineering:
      • WindowPlugin relates to deposit/redemption windows.
      • MaturityVault is a Vault that 'pushes' the assets to the depositing accounts on maturation. BUT it should not know anything of promisedYield or fixedYield as those are FixedYieldVault concerns.
        • Moved _fixedYield to FixedYieldVault.
        • Modified expectedAssetsOnMaturity to be virtual and to have a simple implementation.
  • All test support contracts are now called SimpleX.
    • As @lucasia pointed out, 'Mock' was over-used, and most instances were not actually 'mock's.
    • This change may have gone a bit too far. Opinions, please!
  • All testing source is in .t.sol files. We had .m.sol and .sol files in test.
  • Added utility import shortcuts to remappings.txt
    • So we can import from "@credbull/..." for src, "@script/..." for script code and "@test/..." for `test code.
    • Usage of this is ONLY INTENDED FOR test and script source (where the '../../..' imports are prevalent).

Extent

jplodge-pro and others added 30 commits June 26, 2024 23:05
Use the `export function` syntax in all places, where possible.
…ality itself. Invocation point is irrevelant.
* split params init commit

* forge install: forge-std

v1.8.2

* Initial version of local and dev application configuration

* Add `js-toml` dependency.

* Update `.env.sample` to give clear instructions and remove unnecessary keys.

* Moved DEFAULT_ANVIL_KEY to its singular usage.
Manually altered the wrapping of some lines

* Updated deployment helper JS scripts to use TOML application config.

* Fixed some new solc warnings.

* Now using TOML application config to control deployment.

* Re-submit with auto-formatted changes.

* Proposed changes to the GitHub Actions

* Added decoder logic in read/write response

* refactored handle error logic

* Moving DeployTestVault under test package.

* Updated the env sample files to clearly name the expected Account Addresses/Private Keys.

* Added an env key stub to workaround local deploy problems.

* Added more detail to the documentation.

* Update `.env.sample` to give clear instructions and remove unnecessary keys.

* Moved DEFAULT_ANVIL_KEY to its singular usage.
Manually altered the wrapping of some lines

* Moved the config functionality into it's own module, used by both it's peer modules.

* Corrected a badly rebased Makefile.
Removed the DEFAULT_ANVIL_KEY restored it to configuration files. Also renamed it DEPLOYER_PRIVATE_KEY

* Updated the sample file removing moved config points.

* Sepolia config now uses the TOML file.

* Corrected badly-merged local vault command.

* Add TOML files for all the supported environments.

* Updated to use the correct Supabase config item key.

* Linting fixes

* Corrected the shared config code to be exportable.

* Lint fixes.

* Improved post-load config log comment.

* Agh, another lint change.

* Revert "Proposed changes to the GitHub Actions"

This reverts commit 104ab40.

* Try to fix duplicate IERC20 type import

* Manually remove the duplicate IERC20 definition.  See dependency graph using 'forge tree'

* Changing the github actions to trigger from main and development branch

* Make Readme more executable

* Update contracts git-hub actions script

* Updating testnet Env to deploy to baseSepolia

* Fix supabase URL for testnet

* Moved keys out of toml and into secrets (testnet) or env (local only)

* Add yarn to contracts/ github actions to run it locally

* Add Readme for running github actions locally

* Adding yarn to api github actions to run locally

* First cut of using toml in API, starting with app.controller.ts

* Fix linting issue

* Rename TomlConfig file to tomlConfig.ts

* Remove yarn install step from github actions, some errant failures

* Minor Readme updates to start build

* Fix name for check-env github action

* Issue with yarn install now can't find ./types module.  Reverting some package.json changes

* Fix compile error

* Use the dev env in api github actions.  Testnet missing sentry  keys (amongst others).

* Removed some unused keys from the sample

* Upgrade to `dotenv` 16.4.5 to enable multi-path configuration.
Updated `config.ts` to load local env files hierarchically.

* Change to use `.env` instead of `.env.local`
Re-added a `.env.sample` in `contracts`, as `make` and `forge` use it too.

* Added configuration loading documentation to the Makefile.
Added some console logging for clarity.

* The ETH Config functions were identical, so I re-factored it.
Updated some comments to be more accurate .

* Enabled hierarchical loading of `.env` files.
Removed usage of `.env.local` to be consistent across modules.
Enable nestjs CLI to copy the resource files to the `dist` directory.

* Renamed `.env.local.sample`
Linted formatting changes.

* Edit to include any resource tree and to watch those files too.

* Cleanup imports after linting

* Makefil to call yarn db-export rather than node.  Minor Readme cleanup

* Using TomlConfigService across API, replacing Config.

* Default to local env in TomlConfigService

* Fix github actions triggers

* Get TomlConfigService from the app.

* Changed API to use Github actions testnet secrets

* Update Fly.io deploy token.  Hopefully fix the build

* Update Fly.io deploy token.  Hopefully fix the build

* Readme hint on fly deploy token

* Adding deploy to bitlayerTestnet

* Adding test of tomlConfig.  Minor variable renaming

* Adding test of tomlConfig.  Minor variable renaming

* Renamed toml in contracts package

* Updating github actions to checkout@v4.  Node 16 is deprecated.

* Cleared Keys out of toml - shouldn't be checked in

* Adding Operating Key to github actions to fix the build

* Fix config in checkDb and exporter.  Suppress output of key in makefile

* Change API to use service key.  Anon user doesn't have sufficient access

* Changing a few more to Supabase service key.

* Updatig fly.io config

* Changing api toml paths to make fly.io happy

* Changing api toml paths to make fly.io happy

* Still can't get fly.io to read the toml.  Moving back to where it started

* contract changes in changing vault params

* fixed lint errors

* params change in backend and scripts

* changes in operation script

* fixed upside vault params

* lint fix

* Interface refactoring (#54)

* icredbull removed

* refactored ICredbull structs

* removed ICredbull file

* added IErrors

* type changes in scripts

* new refactored changes

* notification service console logger

* resolved toml and readme conflicts

---------

Co-authored-by: Jonathan Lodge <[email protected]>
Co-authored-by: Ian Lucas <[email protected]>
Updated deposit use case to be more reason-able!
Also be clearer in function
Re-worked all the use cases to use the description functions, to log the 'story' of the use case.
Updated all usages to the simplified notation.
Fixed a test description typo.
Copy link

openzeppelin-code bot commented Jul 2, 2024

Re-structuring and renaming of contracts code.

Generated at commit: 41cf88be923bb52854892fd231c52717c2609a14

🚨 Report Summary

Severity Level Results
Contracts Critical
High
Medium
Low
Note
Total
1
0
0
4
14
19
Dependencies Critical
High
Medium
Low
Note
Total
0
0
0
0
0
0

For more details view the full report in OpenZeppelin Code Inspector

@jplodge-pro jplodge-pro changed the title A cleaner version. Re-structuring and renaming of contracts code. Jul 2, 2024
@jplodge-pro jplodge-pro marked this pull request as ready for review July 2, 2024 07:43
@jplodge-pro jplodge-pro merged commit 13eec92 into main Jul 5, 2024
2 checks passed
@jplodge-pro jplodge-pro deleted the restructure-contracts branch July 5, 2024 09:22
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.

3 participants