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

Add replacedOrder to metadata #60

Merged
merged 1 commit into from
Feb 28, 2024
Merged

Add replacedOrder to metadata #60

merged 1 commit into from
Feb 28, 2024

Conversation

m-lord-renkse
Copy link
Contributor

@m-lord-renkse m-lord-renkse commented Feb 28, 2024

Add a new type of metadata: replacedOrder

This meta-data will allow to specify which order UID to replace when doing a replace order operation.

  • Add new datata type
  • ~Version schema: v1.1.0
  • Build new types and adapt compile.ts to export the new metadata schema
  • Build tests: happy path, not-so-happy path

Validations

  • The metadata is optional, as all other metadatas
  • If present, all the props are mandatory: uid
  • uid is the order UID of the order to replace

Copy link

github-actions bot commented Feb 28, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@m-lord-renkse
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@m-lord-renkse
Copy link
Contributor Author

recheck

@alfetopito
Copy link
Collaborator

_ No description provided. _

Please add a description.

Example: #53

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.

I believe you still need to modify the compile.ts as pointed out in the README (contribute section)

This PR is great, only nitpics!

src/schemas/definitions.json Outdated Show resolved Hide resolved
src/schemas/v1.0.1.json Outdated Show resolved Hide resolved
src/schemas/v1.0.1.json Outdated Show resolved Hide resolved
test/schema.spec.ts Show resolved Hide resolved
@anxolin
Copy link
Contributor

anxolin commented Feb 28, 2024

Also @Mateo-mro , please remember is important to add a good description, because reviewer arrive to PRs from working in something else, and might need to be given some context on the motivation and what to expect from the PR.

One other thing that is missing, is that you build and commit the autogenerated files. For that, you need to build it
image

You will need to install yarn if you haven't done so: https://classic.yarnpkg.com/lang/en/docs/install/

Copy link
Collaborator

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

Can you please take a look at the instructions for adding a new metadata? https://github.com/cowprotocol/app-data?tab=readme-ov-file#contribute

It needs more file changes to properly export all types.

src/schemas/replacedOrder/v0.1.0.json Outdated Show resolved Hide resolved
src/schemas/v1.0.1.json Outdated Show resolved Hide resolved
@m-lord-renkse
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@anxolin
Copy link
Contributor

anxolin commented Feb 28, 2024

@Mateo-mro I see you addressed the comments. Thanks for that!

When you address the comments, if you want another review, make sure you re-request it here:

image

Copy link
Collaborator

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

Very nice, thank you!

I have only one nitpick comment, not blocking

test/schema.spec.ts Outdated Show resolved Hide resolved
validator,
{
...BASE_DOCUMENT,
metadata: { replacedOrder: { } },
Copy link
Contributor

Choose a reason for hiding this comment

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

great! thanks for this

@m-lord-renkse m-lord-renkse merged commit 3290751 into main Feb 28, 2024
6 checks passed
@m-lord-renkse m-lord-renkse deleted the add-replaced-order branch February 28, 2024 14:34
@github-actions github-actions bot locked and limited conversation to collaborators Feb 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants