-
Notifications
You must be signed in to change notification settings - Fork 32
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
Fix SendMsgs, delete Send, and introduce GetMsgResult #106
Conversation
Signed-off-by: Masanori Yoshida <[email protected]>
Signed-off-by: Masanori Yoshida <[email protected]>
Signed-off-by: Masanori Yoshida <[email protected]>
Signed-off-by: Masanori Yoshida <[email protected]>
Signed-off-by: Masanori Yoshida <[email protected]>
Signed-off-by: Masanori Yoshida <[email protected]>
Signed-off-by: Masanori Yoshida <[email protected]>
Signed-off-by: Masanori Yoshida <[email protected]>
e06994b
to
d0e7c5a
Compare
…dermint Signed-off-by: Masanori Yoshida <[email protected]>
Signed-off-by: Masanori Yoshida <[email protected]>
} | ||
|
||
resTx, err := node.Tx(context.TODO(), txHash, false) | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we check if the node enables the transaction indexing like the following:
https://github.com/cosmos/relayer/blob/1bfe06cdec3bdd1e0ba64fafac2b6f39cf495e70/relayer/chains/cosmos/tx.go#L303
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If no checks, I think we should write a comment about this assumption.
core/send.go
Outdated
// SendCheckMsgs is an utility function that executes `Chain::SendMsgs` and checks the execution results of all the messages. | ||
func SendCheckMsgs(chain Chain, msgs []types.Msg) bool { | ||
if _, err := chain.SendMsgs(msgs); err != nil { | ||
GetChainLogger(chain).Error("failed to send msgs", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there is not msgs
in error. Is this intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. I will add them.
|
||
// find tx | ||
resTx, err := c.waitForCommit(msgID.txHash) | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the following check is required here? https://github.com/hyperledger-labs/yui-relayer/pull/106/files#diff-3ebe73b99a006b3bde594abcedd2812ffc28cbdf68ad4fbec70e5be4c6bdc1e8R184
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Even if resTx.TxResult.IsErr() == true
, GetMsgResult
returns (msgResult, nil)
.
At that case, msgResult.Status
returns (false, failureReason)
.
if res.Code == 0 && c.msgEventListener != nil { | ||
|
||
// wait for tx being committed | ||
if resTx, err := c.waitForCommit(res.TxHash); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is GetMsgResult
not called here to avoid unnecessary log parsing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!
Signed-off-by: Masanori Yoshida <[email protected]>
…rns an unrecoverable error Signed-off-by: Masanori Yoshida <[email protected]>
Signed-off-by: Masanori Yoshida <[email protected]>
…x `GetFinalizedMsgResult` using this new function Signed-off-by: Masanori Yoshida <[email protected]>
Signed-off-by: Masanori Yoshida <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks! LGTM👍
No description provided.