Skip to content

Conversation

@jmack33
Copy link

@jmack33 jmack33 commented Jan 26, 2025


title: "[SDK/Dashboard/Portal] Feature/Fix: Concise title for the changes"

If you did not copy the branch name from Linear, paste the issue tag here (format is TEAM-0000):

Notes for the reviewer

Anything important to call out? Be sure to also clarify these in your comments.

How to test

Unit tests, playground, etc.


PR-Codex overview

This PR focuses on enhancing the CounterModule contract by introducing a new BOPTokenModule with additional functionalities for managing token transfers, fees, and storage.

Detailed summary

  • Created BOPTokenModule inheriting from Module.
  • Introduced TokenStorage library for managing token state.
  • Added functions for managing transaction fees, charity percentages, and burn percentages.
  • Implemented token transfer, approval, and allowance mechanisms.
  • Updated getModuleConfig to include new functionalities and storage access.
  • Removed the original CounterModule implementation details.

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

Signed-off-by: jmack33 <[email protected]>
@jmack33 jmack33 requested review from a team as code owners January 26, 2025 00:45
@changeset-bot
Copy link

changeset-bot bot commented Jan 26, 2025

⚠️ No Changeset found

Latest commit: 3c5d392

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

@vercel
Copy link

vercel bot commented Jan 26, 2025

@jmack33 is attempting to deploy a commit to the thirdweb Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions github-actions bot added the Portal Involves changes to the Portal (docs) codebase. label Jan 26, 2025
return CounterStorage.data();
}
}
config.callbackFunctions = new CallbackFunction config.fallbackFunctions = new FallbackFunction ;
Copy link
Contributor

Choose a reason for hiding this comment

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

The array initialization syntax appears malformed. The correct syntax should be:

config.callbackFunctions = new CallbackFunction[](1);
config.fallbackFunctions = new FallbackFunction[](5);

Based on the number of fallback functions defined below, the second array needs space for 5 elements.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

Comment on lines +43 to +45
config.callbackFunctions[0] = CallbackFunction(
this.beforeTransfer.selector
);
Copy link
Contributor

Choose a reason for hiding this comment

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

The beforeTransfer callback is registered in getModuleConfig() but the function itself appears to be missing from the contract. This creates a potential runtime error since the selector references a non-existent function. Either implement the beforeTransfer function with the appropriate hook logic, or remove its registration from the callback configuration.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

@graphite-app
Copy link
Contributor

graphite-app bot commented Jan 26, 2025

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • merge-queue - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

@jmack33
Copy link
Author

jmack33 commented Jan 26, 2025

Thanks

@jmack33 jmack33 closed this Jan 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Portal Involves changes to the Portal (docs) codebase.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant