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

adds serializer for Celo CIP 42 transactions #755

Merged
merged 13 commits into from
Jul 1, 2023

Conversation

aaronmgdr
Copy link
Contributor

@aaronmgdr aaronmgdr commented Jun 21, 2023

followup from #691

A custom Serializer supporting Celo CIP-42 (fee currency) Transactions.


PR-Codex overview

Detailed summary

  • Added serializer for Celo CIP-42 transactions.
  • Imported celoSerializers in src/chains/index.ts.
  • Added serializeTransaction function in src/chains/serializers/celo.ts.
  • Added utility functions and types related to CIP-42 transactions in src/chains/serializers/celo.ts.
  • Added tests for CIP-42 transaction serialization in src/chains/serializers/celo.test.ts.

The following files were skipped due to too many changes: src/chains/serializers/celo.test.ts

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

@changeset-bot
Copy link

changeset-bot bot commented Jun 21, 2023

🦋 Changeset detected

Latest commit: 4648ec4

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

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

@vercel
Copy link

vercel bot commented Jun 21, 2023

Someone is attempting to deploy a commit to the wagmi Team on Vercel.

A member of the Team first needs to authorize it.

@codecov
Copy link

codecov bot commented Jun 21, 2023

Codecov Report

Merging #755 (e589a13) into main (ee93ca4) will decrease coverage by 5.06%.
The diff coverage is 39.89%.

❗ Current head e589a13 differs from pull request most recent head 4648ec4. Consider uploading reports for the commit 4648ec4 to get more accurate results

@@            Coverage Diff             @@
##             main     #755      +/-   ##
==========================================
- Coverage   99.78%   94.72%   -5.06%     
==========================================
  Files         255      253       -2     
  Lines       24460    24491      +31     
  Branches     2171     1865     -306     
==========================================
- Hits        24407    23199    -1208     
- Misses         50     1281    +1231     
- Partials        3       11       +8     
Impacted Files Coverage Δ
src/chains/serializers/celo.ts 38.65% <38.65%> (ø)
src/chains/index.ts 91.36% <100.00%> (+0.25%) ⬆️

... and 50 files with indirect coverage changes

@vercel
Copy link

vercel bot commented Jun 28, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
viem 🔄 Building (Inspect) Jun 28, 2023 8:09pm

@aaronmgdr aaronmgdr marked this pull request as ready for review June 29, 2023 20:28
@jxom
Copy link
Member

jxom commented Jun 29, 2023

Looking great! Just need to get some coverage on these lines:
Screenshot 2023-06-30 at 7 30 56 am

@aaronmgdr
Copy link
Contributor Author

aaronmgdr commented Jun 30, 2023

Looking great! Just need to get some coverage on these lines: Screenshot 2023-06-30 at 7 30 56 am

Ive added test for the lines that were showen as missing. but on CI its saying almost none of the file is covered. 🤔 This is odd since the lines it points to are the entire serializeTransaction and serializeTransactionCIP42 functions.

Also I create a PR on top of the first that creates some transaction argument assertion functions. I thought i might be useful to have these in one place rather than rewriting for each serializer.

https://github.com/clabs-co/viem/pull/2/files

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.

2 participants