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 sentry #43

Merged
merged 4 commits into from
Aug 18, 2023
Merged

Add sentry #43

merged 4 commits into from
Aug 18, 2023

Conversation

anxolin
Copy link
Contributor

@anxolin anxolin commented Aug 16, 2023

Summary

Tenderly Actions logs can't be trusted 100%, they currently have an issue which make us have some blind spots on what is happening.

for this reason i integrated the WatchTower with tenderly, so we can debug better cases we encounter in production:

image

Sentry project: https://cowprotocol.sentry.io/projects/watch-tower-tenderly/?project=4505714328928256

To Test

  • Login in Sentry
  • Observe the test error i manually created

mfw78
mfw78 previously approved these changes Aug 16, 2023
@alfetopito
Copy link

Summary

Tenderly Actions logs can't be trusted 100%, they currently have an issue which make us have some blind spots on what is happening.

for this reason i integrated the WatchTower with tenderly, so we can debug better cases we encounter in production:
image

Sentry project: https://cowprotocol.sentry.io/projects/watch-tower-tenderly/?project=4505714328928256

To Test

* Login in Tenderly

* Observe the test error i manually created

Did you mean: sentry?

alfetopito
alfetopito previously approved these changes Aug 16, 2023
Copy link

@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.

I see it

image

Just cosmetic comments

README.md Outdated Show resolved Hide resolved
actions/utils.ts Outdated
Comment on lines 58 to 61
const transaction = Sentry.startTransaction({
op: "test",
name: "My First Test Transaction",
});

Choose a reason for hiding this comment

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

Will this be removed before merging?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes 100%

@anxolin anxolin dismissed stale reviews from alfetopito and mfw78 via 6f900bb August 16, 2023 12:50
shoom3301
shoom3301 previously approved these changes Aug 16, 2023
alfetopito
alfetopito previously approved these changes Aug 16, 2023
Copy link
Contributor

@mfw78 mfw78 left a comment

Choose a reason for hiding this comment

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

Minor nits

@@ -127,15 +131,49 @@ async function getTradeableOrderWithSignature(
conditionalOrder: ConditionalOrder,
contract: ComposableCoW
) {
const proof = conditionalOrder.proof ? conditionalOrder.proof.path : [];
const offchainInput = "0x";
contract.getTradeableOrderWithSignature;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure of this line?

Comment on lines +137 to +163
const { to, data } =
await contract.populateTransaction.getTradeableOrderWithSignature(
owner,
conditionalOrder.params,
offchainInput,
proof
);

// await contract.provider
// .estimateGas({
// to: contract.address,
// data,
// })
// .then((r) => {
// console.log("[getTradeableOrderWithSignature:estimateGas] Success", r);
// })
// .catch((e) => {
// console.error(
// '[getTradeableOrderWithSignature:estimateGas] Error during "getTradeableOrderWithSignature" call: ',
// e
// );
// });

console.log("[getTradeableOrderWithSignature] Simulate", {
to,
data,
});
Copy link
Contributor

@mfw78 mfw78 Aug 17, 2023

Choose a reason for hiding this comment

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

If the objective is to get the calldata so that it can be tested / simulated elsewhere, may I suggest using encodeFunctionData to produce the calldata that you desire for the simulation?

Actually, on second glance, populateTransaction does seem better. Is there any reason for keeping the estimateGas commented code though as this would return the same errors as just doing the callStatic, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, sorry, this was a test I did debug the issue yesterday. I should have removed it. Im doing a follow-up with some cleanup and a small refactor and I will kill this

} from "@sentry/node";
import { CaptureConsole as CaptureConsoleIntegration } from "@sentry/integrations";

// const Sentry = require("@sentry/node");

Choose a reason for hiding this comment

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

Suggested change
// const Sentry = require("@sentry/node");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will delete in a follow up PR (small refactor)

Comment on lines +145 to +158
// await contract.provider
// .estimateGas({
// to: contract.address,
// data,
// })
// .then((r) => {
// console.log("[getTradeableOrderWithSignature:estimateGas] Success", r);
// })
// .catch((e) => {
// console.error(
// '[getTradeableOrderWithSignature:estimateGas] Error during "getTradeableOrderWithSignature" call: ',
// e
// );
// });

Choose a reason for hiding this comment

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

Suggested change
// await contract.provider
// .estimateGas({
// to: contract.address,
// data,
// })
// .then((r) => {
// console.log("[getTradeableOrderWithSignature:estimateGas] Success", r);
// })
// .catch((e) => {
// console.error(
// '[getTradeableOrderWithSignature:estimateGas] Error during "getTradeableOrderWithSignature" call: ',
// e
// );
// });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will delete in a follow up PR. This was useful for some test, but i can remove it now

@anxolin
Copy link
Contributor Author

anxolin commented Aug 18, 2023

The comments above not addressed in this PR is coming in a follow up today

@anxolin anxolin merged commit ee07f83 into main Aug 18, 2023
2 checks passed
@alfetopito alfetopito deleted the add-sentry branch August 21, 2023 09:21
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.

4 participants