-
Notifications
You must be signed in to change notification settings - Fork 103
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
Merged
Merged
Changes from 20 commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
c6e3045
feat: add proto msgs
aleem1314 ffd1801
add events proto message
aleem1314 3ab532b
feat: add msg implementation
aleem1314 f58dd43
chore: fix tests
aleem1314 b3db5f8
feat: add server tests
aleem1314 f7e1427
Merge branch 'master' into aleem/msg-bridge
aleem1314 cc59bca
chore: make proto-gen
aleem1314 5154273
chore: fix failing tests
aleem1314 c4e3a53
Update proto/regen/ecocredit/v1/events.proto
aleem1314 3ec9dbd
Update proto/regen/ecocredit/v1/tx.proto
aleem1314 733386a
Update x/ecocredit/client/tx.go
aleem1314 769dc29
Update x/ecocredit/core/msg_bridge.go
aleem1314 951d55d
Update proto/regen/ecocredit/v1/tx.proto
aleem1314 77c6685
chore: make proto-gen
aleem1314 4632dec
Update proto/regen/ecocredit/v1/events.proto
aleem1314 28f26d4
Update proto/regen/ecocredit/v1/tx.proto
aleem1314 2d99007
refactor: review changes
aleem1314 4cf6e8a
Update x/ecocredit/client/tx.go
aleem1314 5b24e33
Update x/ecocredit/core/msg_bridge.go
aleem1314 fca1881
Update x/ecocredit/server/testsuite/suite.go
aleem1314 cd90363
chore: review changes
aleem1314 20a47e6
Merge branch 'aleem/msg-bridge' of https://github.com/regen-network/r…
aleem1314 ca89998
Update x/ecocredit/core/msg_bridge.go
aleem1314 f8c3ddd
Merge branch 'master' into aleem/msg-bridge
aleem1314 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Oops, something went wrong.
Large diffs are not rendered by default.
Oops, something went wrong.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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!