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

Stargate integration #120

Merged
merged 96 commits into from
Dec 14, 2020
Merged

Stargate integration #120

merged 96 commits into from
Dec 14, 2020

Conversation

jkilpatr
Copy link
Member

@jkilpatr jkilpatr commented Nov 24, 2020

This pull is a combination of #114, #115, and #118 as such I'll be closing those other pull requests as we try to tile all of this together and get stargate compatible Peggy running.

Obviously compatibility with go-aminio transactions is going away, but while we have it I would like to limit the scope of this already large pull request to the minimum required to get things working.

TODO

  • Get integration tests to fail properly
  • Get go tests working
  • Get integration tests launching the cosmos chain
  • Evaluate GRPC integration in ibc-rs
  • Upgrade Rust code to use GRPC for requests
  • Get everything working with GRPC
  • backwards compatible go-amino transaction formats
  • Protobuf transactions

jackzampolin and others added 30 commits November 16, 2020 10:09
* Update proto files

* Passing testsgit stage .! 🎉
Because -x was not set in all-up-test-internal.sh
the contract deploy operation could fail, fail the
test and not exit with exit code one.

This has caused the tests to look like they
are passing when in fact they are maximum broken.
jackzampolin and others added 7 commits December 8, 2020 17:03
Sending messages with deep_space only works with uint256 as a number.
The previous design was flawed, namely it would check the 'last_checked_block'
twice. Once as the last block in the range and again as the first block in
the range during the next round.

The simple solution is to add one.
Just copy/pasted for now. I get the sense that attestation processing
is broken at some other level though.
This function was at first used to downcast 256bit integers from
ethereum to u64's to send to Cosmos, once it became clear that
we had to send uint256 literals due to problems in deep_space
transaction generation it's now just an overcomplicated overflow
check that needs to be simplified.
We no longer need this message as we've refactored it instead into
MsgDepositClaim and MsgWithdrawClaim.

Sadly MsgDepositClaim does not seem to be execute properly in
the integration test, it does not generate coins. It does on the
other hand seem to work fine in unit tests.
@jkilpatr jkilpatr force-pushed the jkilpatr/stargate-integration branch from b96825b to bd357c0 Compare December 9, 2020 14:54
This patch adds a test case for storing and then reading multiple
votes on an attestation. Currently this test does not pass becuase
the prefix scan required to load attestation details was broken
a few commits ago in 145496d
This commit fixes the issue where claims voted on by multiple
validators would not process properly. This issue was caused
by a misuse of types.Any. If a types.Any value is created and
then passed around it will have a cached value which can be accessed
without unmarshaling the binary value within. Previous iterations of
this code would always overwrite the types.Any value embedded within
an attestation before processing it.

Since the Attestation was keyed on all the claim details that mattered
this was irrelevent to the processing logic but very relevent to the
types.Any unmarshaling logic, which would only check for the cached
value that was always being produced by the new transaction message
being passed in.

This patch simply makes that pattern explicit and has the attestation
call stack simply take the interface value implementing ethereum claim
all the way from the transaction handler down into the processing
of the attestation.
For some reason the string comparison of the new
peggy/<contract address> denom values failed in validate basic this
patch is a fix for that where we just do some other validation first
and take advantage of the broken out eth addresses of the contracts
to great an easier comparison.
This pulls in an upstream fix for sending tokens, specifically getting
account info from accounts with only some tokens and no previous
transactions.
In an effort to better understand my problems with this message I
designed a test that doesn't seem to find anything wrong with it.

This leads me to believe that the error I'm having trouble with is
somewhere in the antehandler before we get the message in our code
or in my own message sending code.
Alex made an effort to generalize bridge signature submissions into
a generic 'bridge signature submission' message. This is mostly just
extra interfaces and complexity because Peggy only needs two different
types of signatures. It's possible adding an arbitrary transaction
endpoint would add a third type of signature, but we're never going
to have dozens of these.

The extra handling logic involved in generalizing the process is simply
not justified.
Due to the previous design of this function we had two opportunities
to crash thanks to an improperly formatted denom string. This patch
adds checks and removes redundant string splitting.
These are all out of date at this point and should be removed before
they confuse anyone.

Included with this patch is a proto rebuild that updates the comments
only in the proto files
Print out the struct to provide some context
These field names got de-synced during a refactor. This patches
that issue and gets us up to submitting batch confirmations, which
are currently not being interpreted correctly it seems.
This patch performs what is probably some un-needed optimization by
parallelizing setting updating the validator eth addresses and sending
orchestrators eth so that they can pay fees.

These two steps stood out to me as taking quite a while, because they
involved waiting for transactions to actually enter the blockchain in
a loop. We don't actually do this too often as most of our testing
involves looking for things the orchestrators are doing in the
background which is already parallel.

The section where the validators send in their ethereum addresses
was pretty straightforward and probably worthwhile. On the other hand
parallelizing the sending of eth from the miner address ran into several
headaches, while it works it's nearly 100 lines and involves building
custom transactions so that they can all co-exist in the Ethereum
mempool despite having a single source address. I've left a comment
indicating that if it ever gives anyone trouble it should be removed.
This patch to Contact displays the raw_log error message from cosmos
as part of the user error message. This is a big improvement in
debugging ergonomics, the previous way of getting this info required
using the trace logging level and carefully specifying the correct
module as a filter statement to the logging library.
The ValidateBasic batch signature verification was obviously not
functional, without the ability to get the peggy id from the keeper
the check can simply no longer be performed at that location like it
was previously.
Minor cosmetic issues only int his patch. Mostly around unclear
or duplicated and not updated error messages and code that
needed improved formatting for better readability.
This patch just cleans up some unused code warnings left over from the
oracle refactor.
This minor function was written and then left unused post Stargate
refactor. I see no harm in adding it back in to further validate
our sorting tests.
A conflict between vim-minimal package and vim-enhanced (which seems to be
the default alias for 'vim' seems to have started after the update to
the Fedora 33 container image, which now contains the minimal vim
rather than simply 'vi'.

This simple patch fixes the issue by specifying only a dependency
on vim-minimal. You could also just remove the requirement assuming
some future version of the image won't revert to vi. Which I don't think
is probable.
@jkilpatr jkilpatr force-pushed the jkilpatr/stargate-integration branch from 83522d7 to 14ff799 Compare December 14, 2020 00:28
@jkilpatr jkilpatr merged commit bfb0241 into master Dec 14, 2020
@jkilpatr jkilpatr deleted the jkilpatr/stargate-integration branch April 11, 2021 11:49
IntegralTeam pushed a commit to IntegralTeam/cosmos-gravity-bridge that referenced this pull request Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants