Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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(ecocredit): custom bridging support for cancelling credits #1101
feat(ecocredit): custom bridging support for cancelling credits #1101
Changes from 8 commits
c6e3045
ffd1801
3ab532b
f58dd43
b3db5f8
f7e1427
cc59bca
5154273
c4e3a53
3ec9dbd
733386a
769dc29
951d55d
77c6685
4632dec
28f26d4
2d99007
4cf6e8a
5b24e33
fca1881
cd90363
20a47e6
ca89998
f8c3ddd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Looking at this again in more detail, I think it's preferable for us to not nest proto messages that are used as GRPC request messages like this. Ideally when defining msg server API endpoints, each can evolve independently (e.g.
Bridge()
andCancel()
should be able to evolve independently). So similar to how we always have explicitly separateMsgBridgeRepsonse
andMsgCancelResponse
, we should probably avoid embedding aMsgCancel
field inside theMsgBridge
message.In terms of which fields from
MsgCancel
we need when a user is callingMsgBridge
, I the following is sufficient:repeated credits
(could use a similar sub-message toCancelCredits
calledBridgeCredits
?)My assumption is that we don't actually need an explicit "reason" in the bridge call, as most of the information we would want to pass in the cancel reason would be describing that it's a bridge action, and possibly what chain its coming from. But all of this info we can just get from the
target
field, and knowing its aBridge()
RPC call.So for filling the "reason" in the cancel event, can we just hardcode it in the message server to set
reason = "bridge"
orreason = "bridge-${target}"
?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.
Just to note, this was implemented as specified in the issue (or at least to our interpretation). That being said, I do agree with the changes proposed. When automatically filling the reason for
MsgCancel
in the message server implementation, I would be in favor of"bridge-${target}"
.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.
sorry abt the confusion!