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

Cowswap332/fix ipfs uploading #31

Merged
merged 6 commits into from
Jun 7, 2022
Merged

Conversation

alfetopito
Copy link
Contributor

Summary

Part of cowprotocol/cowswap#332

Fixing 2 main issues:

  • Ipfs uri set to undefined - default was overwritten in the constructor
  • Split uri into read and write - because there's a different endpoint for each

Also:

  • Updated pinned file name to appData - no longer only affiliate related
  • Added version field as part of quote schema

Testing

  • Unit tests
  • Also tested it locally on CowSwap using yalc

@alfetopito alfetopito self-assigned this Jun 3, 2022
@alfetopito alfetopito requested review from a team June 3, 2022 13:15
@coveralls
Copy link
Collaborator

coveralls commented Jun 3, 2022

Coverage Status

Coverage increased (+2.7%) to 81.059% when pulling 5453256 on cowswap332/fix-ipfs-uploading into f9e5ca3 on main.

@github-actions
Copy link

github-actions bot commented Jun 3, 2022

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

Copy link
Contributor

@W3stside W3stside left a comment

Choose a reason for hiding this comment

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

code looks good

Copy link
Contributor

@ramirotw ramirotw left a comment

Choose a reason for hiding this comment

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

looks good!

the coveralls fail comes from the src/utils/ipfs.ts file you modified and for which we don't have any tests?

@alfetopito
Copy link
Contributor Author

looks good!

the coveralls fail comes from the src/utils/ipfs.ts file you modified and for which we don't have any tests?

IPFS is tested indirectly
Screen Shot 2022-06-07 at 12 03 48

My changes are covered by unit tests, but due to a few extra lines added it's counting as overall coverage decreased

I'll take another look to add more unittests, probably on the context.ts tests

Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

Soft approve

src/utils/context.ts Show resolved Hide resolved
@alfetopito alfetopito force-pushed the cowswap332/fix-ipfs-uploading branch from 7343863 to 5453256 Compare June 7, 2022 16:19
@alfetopito alfetopito merged commit d8f388e into main Jun 7, 2022
@alfetopito alfetopito deleted the cowswap332/fix-ipfs-uploading branch June 7, 2022 16:26
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.

None yet

5 participants