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

feat: Add ts contract package for TS #6260

Merged
merged 13 commits into from
Jul 19, 2023

Conversation

roninjin10
Copy link
Contributor

@roninjin10 roninjin10 commented Jul 11, 2023

  • Add a ts interface for contracts-bedrock
  • use wagmi cli to do it
  • I have contributed to wagmi cli in the past so we should have an easy time making custom plugins or upstreaming changes if we need them long term. The code is pretty simple for others to pick up as well and very well tested.
  • We need this badly internally. We are currently copy pasting abis on the ecopod team and in the sdk because abis imported as JSON do not get correct type inference. This fixes that and.
  • This allows us to smartly configure contract addresses in our utils in a generated way
  • this exports a react interface to our contracts, something optimism never had
  • wagmi actions allow use in javascript with vanilla js or non-react frameworks
  • the constants are exported from the main entrypoint
  • we need to make entrypoints separate or else everybody would be forced to install react, wagmi etc. even if you are just using constants
  • We want to mostly generate this kind of code for the best maintence cost. This should eventually completely replace the SDK if all goes well. Through plugins we can generate anything and then expose a superset to the API when necessary
  • I added a custom plugin to generate an addresses mapping
  • I added a custom plugin to append an eslint ignore
  • We are generating and commiting instead of generating in the build. The generation is not hermetic (it has a time stamp) which means it would bust the nx cache everytime it built. This also makes the build faster.
  • I will add CI in a follow up pr that will check that the config is up to date. I'm working with wagmi to make this a wagmi cli option npx wagmi check
  • The generated code has unnecessary duplication of the abis and the addresses. I will open issue to find a way to fix this as well
  • I am building the package with tsup because it's fast and it builds cjs and esm correctly and handles multiple entrypoints with ease

@changeset-bot
Copy link

changeset-bot bot commented Jul 11, 2023

⚠️ No Changeset found

Latest commit: 3695b73

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.

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

💥 An error occurred when fetching the changed packages and changesets in this PR
Some errors occurred when validating the changesets config:
The package or glob expression "contracts-bedrock" specified in the `linked` option does not match any package in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
The package or glob expression "contracts-ts" specified in the `linked` option does not match any package in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.

@roninjin10
Copy link
Contributor Author

roninjin10 commented Jul 11, 2023

@semgrep-app
Copy link
Contributor

semgrep-app bot commented Jul 11, 2023

Semgrep found 1 nondeterministic-select finding:

  • op-proposer/proposer/l2_output_submitter.go: L350-375

Logic executed as a result of ticker ticker may execute more times than desired.
When both ticker and l.done are written to at the same time, the scheduler randomly picks a
case to execute. As a result, the ticker.C may excute one more time than expected.

Ignore this finding from nondeterministic-select.

@roninjin10 roninjin10 marked this pull request as draft July 11, 2023 20:34
@roninjin10 roninjin10 force-pushed the 07-11-feat_Add_ts_contract_package_for_TS branch from 8c243c7 to 1285465 Compare July 11, 2023 21:51
Base automatically changed from 07-11-fix_sdk_Add_missing_message_statuses to develop July 11, 2023 22:01
@roninjin10 roninjin10 force-pushed the 07-11-feat_Add_ts_contract_package_for_TS branch from 1285465 to ed63b46 Compare July 12, 2023 04:33
@netlify
Copy link

netlify bot commented Jul 12, 2023

Deploy Preview for opstack-docs canceled.

Name Link
🔨 Latest commit 3695b73
🔍 Latest deploy log https://app.netlify.com/sites/opstack-docs/deploys/64b86209f0851d000826829c

@roninjin10 roninjin10 marked this pull request as ready for review July 12, 2023 04:45
@codecov
Copy link

codecov bot commented Jul 12, 2023

Codecov Report

Merging #6260 (3695b73) into develop (4604825) will decrease coverage by 3.75%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #6260      +/-   ##
===========================================
- Coverage    44.88%   41.14%   -3.75%     
===========================================
  Files          450      452       +2     
  Lines        29347    32673    +3326     
  Branches       748     1914    +1166     
===========================================
+ Hits         13173    13443     +270     
- Misses       15100    17763    +2663     
- Partials      1074     1467     +393     
Flag Coverage Δ
bedrock-go-tests 43.74% <ø> (+0.01%) ⬆️
cannon-go-tests 62.40% <ø> (ø)
common-ts-tests 26.82% <ø> (ø)
contracts-bedrock-tests 63.36% <ø> (+3.45%) ⬆️
contracts-ts-tests 6.88% <ø> (?)
core-utils-tests 44.24% <ø> (ø)
fault-detector-tests 26.95% <ø> (ø)
sdk-next-tests 42.48% <ø> (ø)
sdk-tests 42.48% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
op-bindings/predeploys/addresses.go 100.00% <ø> (ø)
packages/contracts-ts/src/constants.ts 100.00% <ø> (ø)
packages/contracts-ts/src/react.ts 3.58% <ø> (ø)

... and 5 files with indirect coverage changes

@roninjin10 roninjin10 force-pushed the 07-11-feat_Add_ts_contract_package_for_TS branch from ed63b46 to f4a25b6 Compare July 12, 2023 07:58
@roninjin10 roninjin10 force-pushed the 07-11-feat_Add_ts_contract_package_for_TS branch 3 times, most recently from f0af6c9 to 6431ffb Compare July 12, 2023 08:09
@roninjin10 roninjin10 force-pushed the 07-11-feat_Add_ts_contract_package_for_TS branch 9 times, most recently from a6d27fe to a059e7d Compare July 12, 2023 23:05
nx.json Show resolved Hide resolved
packages/contracts-ts/CODE_GEN.md Outdated Show resolved Hide resolved
packages/contracts-ts/CODE_GEN.md Outdated Show resolved Hide resolved
packages/contracts-ts/package.json Outdated Show resolved Hide resolved
packages/contracts-ts/package.json Outdated Show resolved Hide resolved
packages/contracts-ts/package.json Show resolved Hide resolved
packages/contracts-ts/package.json Outdated Show resolved Hide resolved
packages/contracts-ts/package.json Outdated Show resolved Hide resolved
packages/contracts-ts/package.json Outdated Show resolved Hide resolved
packages/contracts-ts/package.json Outdated Show resolved Hide resolved
@roninjin10 roninjin10 force-pushed the 07-11-feat_Add_ts_contract_package_for_TS branch from a48f2e8 to 6f2374f Compare July 18, 2023 21:53
@mergify mergify bot removed the conflict label Jul 18, 2023
@mergify
Copy link
Contributor

mergify bot commented Jul 19, 2023

Hey @roninjin10! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot added the conflict label Jul 19, 2023
Will Cory added 12 commits July 19, 2023 12:45
different package nah

do it right

Update packages/contracts-ts/CODE_GEN.md

Update packages/contracts-ts/CODE_GEN.md

Update packages/contracts-ts/package.json

Update packages/contracts-ts/package.json

Update packages/contracts-ts/package.json

Update packages/contracts-ts/package.json

Update packages/contracts-ts/package.json

Update packages/contracts-ts/wagmi.config.ts

Update packages/contracts-ts/wagmi.config.ts

feat: generate abis too

feat: Update op-bindings README

feat: Add lint

feat: Add circleci

fix: Fix deploy config

fix: Add coverage

fix: linter

self review nits

fix: Test was not doing what it thought it was doing

linter
@roninjin10 roninjin10 force-pushed the 07-11-feat_Add_ts_contract_package_for_TS branch from 6f2374f to f0be885 Compare July 19, 2023 19:46
@mergify mergify bot removed the conflict label Jul 19, 2023
@mergify
Copy link
Contributor

mergify bot commented Jul 19, 2023

This PR has been added to the merge queue, and will be merged soon.

@mergify
Copy link
Contributor

mergify bot commented Jul 19, 2023

This PR is next in line to be merged, and will be merged as soon as checks pass.

1 similar comment
@mergify
Copy link
Contributor

mergify bot commented Jul 19, 2023

This PR is next in line to be merged, and will be merged as soon as checks pass.

@OptimismBot OptimismBot merged commit 836ec28 into develop Jul 19, 2023
@OptimismBot OptimismBot deleted the 07-11-feat_Add_ts_contract_package_for_TS branch July 19, 2023 22:36
@mergify mergify bot removed the on-merge-train label Jul 19, 2023
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.

9 participants