Feature: oft adapter step1 - basic upgradeable adapter/minterburner

[blueogin]

Description

This is the PR for OFT step1 - Write upgradeable GoodDollarMinterBurner and GoodDollarOFTAdapter contracts with basic functionalities

About #7

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

• PR title matches follow: (Feature|Bug|Chore) Task Name
• My code follows the style guidelines of this project
• I have followed all the instructions described in the initial task (check Definitions of Done)
• I have performed a self-review of my own code
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• I have added reference to a related issue in the repository
• I have added a detailed description of the changes proposed in the pull request. I am as descriptive as possible, assisting reviewers as much as possible.
• I have added screenshots related to my pull request (for frontend tasks)
• I have pasted a gif showing the feature.
• @mentions of the person or team responsible for reviewing proposed changes

[sourcery-ai]

Hey - I've found 3 issues, and left some high level feedback:

• In GoodDollarOFTAdapter._credit the requestId is derived from keccak256(..., block.timestamp), which makes it impossible to know and pre-approve via approveRequest before the message is processed; consider passing a deterministic ID from the source chain or otherwise reworking the approval flow so approvedRequests is actually usable.
• BridgeFees.minFee and BridgeFees.maxFee are stored but never used in _takeFee or elsewhere, so the effective fee logic only depends on fee bps; either wire in min/max bounds enforcement or remove these fields to avoid confusion.
• In GoodDollarMinterBurner, the initialize signature uses INameService but that interface is not defined/imported in this file (while an unused IIdentity interface is declared), which will prevent compilation; align the imported/declared interfaces with the types actually used.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `GoodDollarOFTAdapter._credit` the `requestId` is derived from `keccak256(..., block.timestamp)`, which makes it impossible to know and pre-approve via `approveRequest` before the message is processed; consider passing a deterministic ID from the source chain or otherwise reworking the approval flow so `approvedRequests` is actually usable.
- `BridgeFees.minFee` and `BridgeFees.maxFee` are stored but never used in `_takeFee` or elsewhere, so the effective fee logic only depends on `fee` bps; either wire in min/max bounds enforcement or remove these fields to avoid confusion.
- In `GoodDollarMinterBurner`, the `initialize` signature uses `INameService` but that interface is not defined/imported in this file (while an unused `IIdentity` interface is declared), which will prevent compilation; align the imported/declared interfaces with the types actually used.

## Individual Comments

### Comment 1
<location> `packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol:132` </location>
<code_context>
+    function initialize(
+        address _token,
+        IMintableBurnable _minterBurner,
+        address _lzEndpoint,
+        address _owner,
+        address _feeRecipient,
</code_context>

<issue_to_address>
**issue:** The `_lzEndpoint` initializer parameter is unused and may mislead callers about configurability.

In `initialize`, `_lzEndpoint` is accepted but never used. Since the endpoint is already set via `OFTCoreUpgradeable`’s constructor, this parameter has no effect yet suggests per-proxy configurability. Either pass `_lzEndpoint` through to the appropriate parent initializer (if available) or remove it to prevent confusion and misconfiguration.
</issue_to_address>

### Comment 2
<location> `packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol:348-353` </location>
<code_context>
+        
+        // Generate a request ID from the message (using source chain ID and amount as a simple hash)
+        // In a real implementation, you might want to pass this from the LayerZero message
+        uint256 requestId = uint256(keccak256(abi.encode(_srcEid, _to, _amountLD, block.timestamp)));
+        
+        // Enforce limits before processing
+        // Note: We track limits by recipient address (_to) since OFT doesn't provide original sender in _credit
+        // For approved requests, call approveRequest() with the requestId before the transfer
+        _enforceLimits(_to, _to, _amountLD, requestId);
+        
+        // Calculate fee if fee recipient is set and fee is configured
</code_context>

<issue_to_address>
**issue (bug_risk):** The approval mechanism using `requestId` derived from `block.timestamp` is not practically usable for pre-approvals.

In `_credit`, `requestId` is computed from `_srcEid`, `_to`, `_amountLD`, and `block.timestamp`, while `_enforceLimits` assumes callers can pre-approve via `approveRequest(requestId)`. Since `block.timestamp` is only known at execution, off-chain systems can’t precompute this ID and pre-approval is impractical even on-chain. This makes the `approvedRequests` bypass effectively non-functional. Consider: (a) passing a deterministic request ID from the LayerZero payload, (b) using an explicit override flag controlled by a privileged role on the destination, or (c) removing this approval path to avoid a misleading control surface.
</issue_to_address>

### Comment 3
<location> `packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol:362-363` </location>
<code_context>
+        }
+        
+        // Mint tokens to recipient (amount minus fee)
+        uint256 amountToRecipient = _amountLD - fee;
+        minterBurner.mint(_to, amountToRecipient);
+        
+        // Mint fee to fee recipient if fee exists
</code_context>

<issue_to_address>
**issue (bug_risk):** The return value of `minterBurner.mint` is ignored, which can hide mint failures while the bridge flow continues.

In `_credit`, the `bool` returned by `minterBurner.mint(_to, amountToRecipient)` is ignored. If `mint` fails and returns `false`, the bridge will still report success and return `amountToRecipient`, desynchronizing accounting from the actual token supply. This should either revert on failure in `GoodDollarMinterBurner.mint` or explicitly check the return value here and revert with a clear error if it is `false`.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[openzeppelin-code]

Feature: oft adapter step1 - basic upgradeable adapter/minterburner

Generated at commit: 867eed0cdad697068992f8da7344ac3e437c0671

🚨 Report Summary

	Severity Level	Results
Contracts	Critical High Medium Low Note Total	0 1 0 9 36 46
Dependencies	Critical High Medium Low Note Total	0 0 0 0 0 0

---

For more details view the full report in OpenZeppelin Code Inspector

[sirpy]

no need daocontract has nativeToken() method to get the address of the G$ token

[blueogin]

Yes I will use nativeToken method

[sirpy]

these should not be requried

[blueogin]

Yes I will remove them

[sirpy]

are all these required?

[blueogin]

they are all required

[sirpy]

shouldnt be disabled or undo the package upgrade you did

[blueogin]

I will uncomment/make it work