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

Forward parsed fullAppData #1448

Merged
merged 3 commits into from
Apr 23, 2024
Merged

Forward parsed fullAppData #1448

merged 3 commits into from
Apr 23, 2024

Conversation

fmrsabino
Copy link
Collaborator

Summary

Adds the parsed App Data to the OrderInfo thus making it available in CowSwapConfirmationView and SwapOrderTransactionInfo.

This can be specially useful to the clients if access to other order properties is required (e.g. slippageBips)

Changes

  • Validates the JSON returned in fullAppData.
  • Forwards the parsed fullAppData via CowSwapConfirmationView and SwapOrderTransactionInfo.

@fmrsabino fmrsabino self-assigned this Apr 23, 2024
@fmrsabino fmrsabino requested a review from a team as a code owner April 23, 2024 09:05
@coveralls
Copy link

coveralls commented Apr 23, 2024

Pull Request Test Coverage Report for Build 8798247876

Details

  • 7 of 9 (77.78%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.03%) to 92.316%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/routes/transactions/entities/confirmation-view/confirmation-view.entity.ts 1 2 50.0%
src/routes/transactions/entities/swaps/swap-order-info.entity.ts 1 2 50.0%
Totals Coverage Status
Change from base Build 8797455664: -0.03%
Covered Lines: 6965
Relevant Lines: 7283

💛 - Coveralls

'{}',
'null',
'{\n "version": "0.1.0",\n "appCode": "Yearn",\n "metadata": {\n "referrer": {\n "version": "0.1.0",\n "address": "0xFEB4acf3df3cDEA7399794D0869ef76A6EfAff52"\n }\n }\n}\n',
])('is valid', (fullAppData) => {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: the value is not used in the description text.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 5c1521c

Comment on lines 142 to 148
it.each(['a', 'a : b', '{', '['])('is not valid', (fullAppData) => {
const order = orderBuilder().with('fullAppData', fullAppData).build();

const result = OrderSchema.safeParse(order);

expect(result.success).toBe(false);
});
Copy link
Member

Choose a reason for hiding this comment

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

This is not in the describe block.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 5c1521c

});
});

it.each(['a', 'a : b', '{', '['])('is not valid', (fullAppData) => {
Copy link
Member

Choose a reason for hiding this comment

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

As above regarding the value not in the description text.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 5c1521c

Comment on lines 96 to 97
.nullish()
.default(null),
Copy link
Member

Choose a reason for hiding this comment

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

As these are not before the transform, null and undefined fullAppData doesn't pass validation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 5c1521c

@@ -101,6 +101,8 @@ export class CowSwapConfirmationView implements Baseline, OrderInfo {
})
executedSurplusFee: string | null;

fullAppData: Record<string, unknown> | null;
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to add an ApiPropertyOptional (with description) for this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 5c1521c

@@ -90,6 +91,8 @@ export class SwapOrderTransactionInfo
})
executedSurplusFee: string | null;

fullAppData: Record<string, unknown> | null;
Copy link
Member

Choose a reason for hiding this comment

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

As before regarding an ApiPropertyOptional.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 5c1521c

@fmrsabino fmrsabino requested a review from iamacook April 23, 2024 09:37
@fmrsabino fmrsabino merged commit 93ad665 into main Apr 23, 2024
16 checks passed
@fmrsabino fmrsabino deleted the forward-app-data branch April 23, 2024 09:49
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

3 participants