-
Notifications
You must be signed in to change notification settings - Fork 237
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
embed gravity bridge module #11
Conversation
// AddrLen is the allowed length (in bytes) for an address. | ||
// | ||
// NOTE: In the SDK, the default value is 255. | ||
AddrLen = 20 |
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.
why changing this? in 0.43, there are 32-byte addresses (e.g. if one uses a different signing algorithm)?
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.
https://github.com/PeggyJV/gravity-bridge/blob/main/module/app/app.go#L104
gravity seems to require this, not sure if it works without it. I guess it need to map any potential cosmos address to ethereum.
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.
Opened issue to gravity, I guess it's possible to only do the verification at module level.
Codecov Report
@@ Coverage Diff @@
## main #11 +/- ##
======================================
Coverage ? 3.75%
======================================
Files ? 12
Lines ? 1145
Branches ? 0
======================================
Hits ? 43
Misses ? 1101
Partials ? 1 Continue to review full report at Codecov.
|
8881053
to
875eb01
Compare
go.mod
Outdated
@@ -24,3 +25,5 @@ require ( | |||
replace google.golang.org/grpc => google.golang.org/grpc v1.33.2 | |||
|
|||
replace github.com/gogo/protobuf => github.com/regen-network/protobuf v1.3.3-alpha.regen.1 | |||
|
|||
replace github.com/99designs/keyring => github.com/cosmos/keyring v1.1.7-0.20210622111912-ef00f8ac3d76 |
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 we expect more dashes in the chain id, then perhaps using this:
https://github.com/crypto-org-chain/keyring/commits/v1.1.6-fixes which includes @leejw51crypto 's patch for the linux secret service in keyring
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.
extracted the keyring change to standalone PR.
cmd/cronosd/cmd/gentx.go
Outdated
} | ||
|
||
aTx, err := clientCtx.TxConfig.TxJSONDecoder()(bz) | ||
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.
might just return aTx, err
directly instead of check err
here.
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.
fixed, thanks
scripts/devnet.yaml
Outdated
@@ -1,5 +1,5 @@ | |||
chainmaind-777: | |||
cmd: ./build/ethermintd | |||
cronos-777: |
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.
missing d
?
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.
hmm, since it's chain id here, maybe we shouldn't add the d
, I removed the d
in the other chain-ids.
263e287
to
0c345a1
Compare
@@ -11,6 +11,19 @@ | |||
"url": "https://github.com/tweag/gomod2nix/archive/67f22dd738d092c6ba88e420350ada0ed4992ae8.tar.gz", | |||
"url_template": "https://github.com/<owner>/<repo>/archive/<rev>.tar.gz" | |||
}, | |||
"gravity-bridge": { | |||
"branch": "main", | |||
"description": "A CosmosSDK application for moving assets on and off of EVM based, POW chains", |
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.
gravity bridge supports only Cosmos <-> Ethereum at the moment
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.
I think it means transfer to and from EVM chain?
The embed on the cosmos side seems fine, I've managed call In the newer commit I changed the
I also added scripts to help to setup gravity locally, the procedure is roughly like this:
|
Closes #9 - add gravity module to app - verify address format at application level - fix pystarport config files - build orchestrator in nix - add a gravity hook implementation template in cronos module - add gravity custom gentx command fix chain id and err handling code add eth_keys command
since we can always do set-delegate-keys later
Simulation test succeed, the module embedding itself should be good, I'm merging this one now. |
Closes #9
replace keyring with patched versionextracted to separated PRTesting will be done in another PR.
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
PR Checklist:
make
)make test
)go fmt
)golangci-lint run
)go list -json -m all | nancy sleuth
)Thank you for your code, it's appreciated! :)