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.

Sign up for GitHub

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

Jump to bottom

Store transaction errors caught before broadcast #936

Merged
merged 7 commits into from
Nov 26, 2024
Merged

Store transaction errors caught before broadcast #936

merged 7 commits into from
Nov 26, 2024

Conversation

amit-momin
Copy link
Contributor

@amit-momin amit-momin commented Nov 20, 2024 •
edited by jira bot
Loading

NONEVM-957

  • Introduced a new OnPrebroadcastError method to the in-memory storage to directly put transactions into the errored state. Previously a transaction had to transition from another state to errored.
  • Separated error parsing and status transition methods so errors can be more easily categorized and handled in both simulation or confirmation
  • Introduced a smaller struct for storing finalized or errored statuses to minimize memory usage in cases of long retention timeouts
  • Introduced a new FatallyErrored status to classify specific Solana errors that should not be retried by plugin side code. Changes to parse and classify errors with this status will be introduced in a later update.

@amit-momin amit-momin marked this pull request as ready for review November 20, 2024 19:26
@amit-momin amit-momin requested a review from a team as a code owner November 20, 2024 19:26
Farber98
Farber98 previously approved these changes Nov 21, 2024
View reviewed changes
Copy link
Contributor

@Farber98 Farber98 left a comment

Choose a reason for hiding this comment

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

LGTM. Nice job with the optimizations while developing this. Left some comments but nothing major

pkg/solana/txm/pendingtx.go Outdated Show resolved Hide resolved
pkg/solana/txm/pendingtx.go Outdated Show resolved Hide resolved
pkg/solana/txm/pendingtx.go Show resolved Hide resolved
pkg/solana/txm/pendingtx_test.go Show resolved Hide resolved
pkg/solana/txm/txm.go Show resolved Hide resolved
@Farber98 Farber98 mentioned this pull request Nov 21, 2024
Closed
silaslenihan
silaslenihan reviewed Nov 21, 2024
View reviewed changes
pkg/solana/txm/pendingtx.go Outdated Show resolved Hide resolved
pkg/solana/txm/txm.go Show resolved Hide resolved
pkg/solana/txm/utils.go Show resolved Hide resolved
pkg/solana/txm/txm.go
}
txm.lggr.Errorw("simulate: unrecognized error", logValues...)
txm.lggr.Errorw("unrecognized error", logValues...)
return Errored, errType
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be fatally errored if it's simulation type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I'll check with CCIP to see what they think. I was sort of following the EVM TXM philosophy of keeping unknown errors retryable but surfacing it through logs. But we can tackle this with the later update for fatal errors

silaslenihan
silaslenihan previously approved these changes Nov 21, 2024
View reviewed changes
@amit-momin amit-momin dismissed stale reviews from silaslenihan and Farber98 via c299869 November 21, 2024 17:48
Farber98
Farber98 previously approved these changes Nov 21, 2024
View reviewed changes
silaslenihan
silaslenihan previously approved these changes Nov 22, 2024
View reviewed changes
Farber98
Farber98 previously approved these changes Nov 22, 2024
View reviewed changes
@amit-momin amit-momin dismissed stale reviews from Farber98 and silaslenihan via 36e1adf November 25, 2024 16:00
@aalu1418 aalu1418 enabled auto-merge (squash) November 26, 2024 14:33
@cl-sonarqube-production
Copy link

Quality Gate passed Quality Gate passed

Issues
1 New issue
1 Fixed issue
0 Accepted issues

Measures
0 Security Hotspots
88.9% Coverage on New Code
0.9% Duplication on New Code

See analysis details on SonarQube

@aalu1418 aalu1418 merged commit e68ee4c into develop Nov 26, 2024
35 checks passed
@aalu1418 aalu1418 deleted the feature/store-prebroadcast-errored-tx branch November 26, 2024 15:02
@Farber98 Farber98 mentioned this pull request Nov 26, 2024
Merged
@amit-momin amit-momin mentioned this pull request Dec 3, 2024
Merged
dhaidashenko pushed a commit that referenced this pull request Dec 20, 2024
@amit-momin @dhaidashenko
Store transaction errors caught before broadcast (#936)
ef7b492 
* Enabled TXM to store error statuses for transactions caught before broadcast

* Addressed feedback

* Removed id from finished tx metadata to reduce memory footprint

* Updated logs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aalu1418 aalu1418 aalu1418 approved these changes

@Farber98 Farber98 Farber98 approved these changes

@silaslenihan silaslenihan silaslenihan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@amit-momin @silaslenihan @Farber98 @aalu1418