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

TXM In-memory: step 3-02-CreateTransaction #12181

Merged
merged 15 commits into from
Apr 1, 2024
Merged

TXM In-memory: step 3-02-CreateTransaction #12181

merged 15 commits into from
Apr 1, 2024

Conversation

poopoothegorilla
Copy link
Contributor

@poopoothegorilla poopoothegorilla commented Feb 26, 2024 •
edited
Loading

NOTES:

  • implements the CreateTransaction Method for the In-Memory Transaction Store
  • Implements the basic CreateTransaction Tests to ensure parity between the In-memory and evm store

Parent PR:

@github-actions GitHub Actions
Copy link
Contributor

I see that you haven't updated any CHANGELOG files. Would it make sense to do so?

This was referenced Feb 26, 2024
Closed
Closed
@poopoothegorilla poopoothegorilla added wip and removed wip labels Feb 27, 2024
@poopoothegorilla poopoothegorilla marked this pull request as ready for review March 21, 2024 19:37
@poopoothegorilla poopoothegorilla requested a review from a team as a code owner March 21, 2024 19:37
@cl-sonarqube-production
Copy link

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
86.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

DylanTinianov
DylanTinianov reviewed Apr 1, 2024
View reviewed changes
common/txmgr/inmemory_store.go
return txmgrtypes.Tx[CHAIN_ID, ADDR, TX_HASH, BLOCK_HASH, SEQ, FEE]{}, nil
tx := txmgrtypes.Tx[CHAIN_ID, ADDR, TX_HASH, BLOCK_HASH, SEQ, FEE]{}
if ms.chainID.String() != chainID.String() {
panic("invalid chain ID")
Copy link
Contributor

@DylanTinianov DylanTinianov Apr 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: Might be useful to improve these error messages across all functions in case the panic occurs. Something like the following?

Suggested change
panic("invalid chain ID")
panic(fmt.Sprintf("invalid chain ID: expected %s but recieved %s", ms.chainID.String(), chainID.String())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

linked this in #12176 so we can address it to the other panics as well

DylanTinianov
DylanTinianov approved these changes Apr 1, 2024
View reviewed changes
Copy link
Contributor

@DylanTinianov DylanTinianov 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, small nit about panic messages

@poopoothegorilla poopoothegorilla merged commit ef5b26a into jtw/step-3-02 Apr 1, 2024
110 of 114 checks passed
@poopoothegorilla poopoothegorilla deleted the jtw/step-3-02-create-transaction branch April 1, 2024 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DylanTinianov DylanTinianov DylanTinianov approved these changes

@amit-momin amit-momin amit-momin 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.
3 participants
@poopoothegorilla @DylanTinianov @amit-momin