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

feat: support concurrent transaction handling #67

Merged
merged 16 commits into from
Jun 21, 2022

Conversation

MonikaCat
Copy link
Contributor

Description

Fixes #64

Checklist

  • Targeted PR against correct branch.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote unit tests.
  • Re-reviewed Files changed in the Github PR explorer.

@MonikaCat MonikaCat marked this pull request as ready for review May 11, 2022 13:14
@MonikaCat MonikaCat added the automerge Automatically merge PR once all prerequisites pass label May 11, 2022
Copy link
Contributor

@huichiaotsou huichiaotsou left a comment

Choose a reason for hiding this comment

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

tested Ack

@MonikaCat MonikaCat requested a review from RiccardoM May 25, 2022 14:01
parser/worker.go Outdated Show resolved Hide resolved
parser/worker.go Outdated Show resolved Hide resolved
parser/worker.go Outdated Show resolved Hide resolved
parser/worker.go Outdated Show resolved Hide resolved
parser/worker.go Outdated Show resolved Hide resolved
parser/worker.go Outdated
// handleMessages accepts the transaction and handles messages contained
// inside the transaction. An error is returned if the message unpacking
// or calling handlers fails.
func (w Worker) handleMessages(tx *types.Tx) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this method can be updated to be more similar to handleTx. We just need to un pack the messages first

It would be something like

// inside ExportTxs
sdkMsgs := make(sdk.Msg[], len(tx.Body.Messages)
for i, stdMsg := range tx.Body.Messages {
  var stdMsg sdk.Msg
  err := w.codec.UnpackAny(msg, &stdMsg)
  if err != nil {
    return err 
  }
  sdkMsgs[i] = sdkMsg
}

for i, sdkMsg := range sdkMsgs {
  go w.handleMessage(i, sdkMsg, tx)
}

And then the handleMessage would be

func (w Woker) handleMessage(index int, tx *types.Tx, msg sdk.Msg) {
  for _, module := range w.modules {
    if messageModule, ok := module.(modules.MessageModule); ok {
      err = messageModule.HandleMsg(i, stdMsg, tx)
      if err != nil {
        w.logger.MsgError(module, tx, stdMsg, err)
      }
    }
  }
}

@MonikaCat
Copy link
Contributor Author

Hey @RiccardoM thanks for that, I updated the code now 👍🏻 Can you review it again pls? thx

@RiccardoM
Copy link
Contributor

Can someone test this code before finally approving it? Maybe @huichiaotsou

Copy link
Contributor

@RiccardoM RiccardoM left a comment

Choose a reason for hiding this comment

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

Code looks good, we just need to test this to make sure it works properly

@mergify mergify bot merged commit d0d22cf into cosmos/v0.44.x Jun 21, 2022
@mergify mergify bot deleted the monika/concurrent-txs branch June 21, 2022 07:06
RiccardoM pushed a commit that referenced this pull request Jul 27, 2022
## Description

Fixes #64

## Checklist
- [x] Targeted PR against correct branch.
- [x] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [ ] Wrote unit tests.  
- [x] Re-reviewed `Files changed` in the Github PR explorer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Automatically merge PR once all prerequisites pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support concurrent transaction handling
3 participants