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

Added the handling of begin and end block events #189

Merged
merged 47 commits into from
Jun 24, 2021
Merged

Added the handling of begin and end block events #189

merged 47 commits into from
Jun 24, 2021

Conversation

RiccardoM
Copy link
Contributor

@RiccardoM RiccardoM commented May 25, 2021

This PR adds the support for being_block_events and end_block_events. Closes #188.

The code has been tested locally on a Desmos <-> Band IBC connection and it works properly.

Note

The code currently relies on a fork of the cosmjs/tendermint-rpc package that is available here. This is done because the NPM cosmjs package does not support the required /block_events endpoint. I've already opened a PR towards their repo to implement this feature (see cosmos/cosmjs#810). Until that one is merged, however, we need to rely on the fork.

@changeset-bot
Copy link

changeset-bot bot commented May 25, 2021

🦋 Changeset detected

Latest commit: a502458

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@confio/relayer Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@ethanfrey ethanfrey self-requested a review May 25, 2021 11:33
Copy link
Contributor

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice contribution. Thank you very much.

It is obviously blocked on cosmos/cosmjs#810 being merged and there are a few cleanup items to do, but in general the code looks good. I need to ensure it doesn't break the CI and ideally would like to add a cases for:

  • Querying still working with v0.41.x (this is broken in the PR, suggestions to fix somehow)
  • Relaying handles events in a "positive test case" (eg. desmos -> band)

I can take care of setting up the CI to test this, but I would need minimally docker images for both chains as well as a list of setup commands you you to establish this channel and trigger packet creation, so we can verify some endblock transactions properly relayed.

I am fine with doing the CI tests in a separate PR (once both chains are tagged), but it is important to have some test there to prevent future regressions.

package.json Outdated
@@ -35,7 +35,7 @@
"@cosmjs/proto-signing": "0.25.3",
"@cosmjs/stargate": "0.25.3",
"@cosmjs/stream": "0.25.3",
"@cosmjs/tendermint-rpc": "0.25.3",
"@cosmjs/tendermint-rpc": "https://gitpkg.now.sh/desmos-labs/cosmjs/packages/tendermint-rpc?riccardo/add-block-search",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, very nice. I never knew how to import non-tagged releases from lerna monorepos

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. But where is the TS -> JS compiled?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@webmaster128 I think you can read more here. It should be compiled by them

maxHeight,
}: QueryOpts = {}): Promise<PacketWithMetadata[]> {
const txsPackets = await this.getPacketsFromTxs({ minHeight, maxHeight });
const eventsPackets = await this.getPacketsFromBlockEvents({ minHeight, maxHeight });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will throw an unspecified (and unhandled) error if used on any chain less than tendermint v0.34.9 (cosmos-sdk v0.42.4). I would tend to making this more robust, unless there really is no need to handle earlier chains.

See comments on cosmos/cosmjs#810 (review)

src/lib/utils.ts Outdated
@@ -96,7 +97,7 @@ export function timestampFromDateNanos(
const nanos = (date.getTime() % 1000) * 1000000 + (date.nanoseconds ?? 0);
return Timestamp.fromPartial({
seconds: new Long(date.getTime() / 1000),
nanos,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of small changes from your editor/prettier config.

Can you run yarn fix to bring this into a standard format? (Maybe we need to add an explicit prettier config for the editors.

src/lib/utils.ts Outdated
},
innerSpec: {
childOrder: [0, 1],
minPrefixLength: 1,
maxPrefixLength: 1,
childSize: 32,
hash: HashOp.SHA256,
},
hash: HashOp.SHA256
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: I think I exposed these in the most recent ics23 package. I should see if I can import them instead of recreating them here.

src/lib/utils.ts Outdated
@@ -1,9 +1,10 @@
import { fromUtf8, toHex, toUtf8 } from '@cosmjs/encoding';
import { BroadcastTxFailure, Coin, logs, StdFee } from '@cosmjs/stargate';
import { parseEvent } from '@cosmjs/stargate/build/logs';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Importing from the build dir smells funny.
This is kind of exposed top-level

I think you need :

import { logs } from '@cosmjs/stargate';
const { parseEvent } = logs;

}))
}));

const flatEvents = ([] as ParsedEvent[]).concat(allEvents.map(parseEvent));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In any case, nice you do parseEvents to do all the validity checks

@ethanfrey
Copy link
Contributor

Oh yeah, you should run yarn changeset to add a line about what feature this adds (it can be a patch release)

@RiccardoM
Copy link
Contributor Author

@ethanfrey I should have fixed everything and added the change set as well

Copy link
Contributor

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks clean. My main concerns (besides CI integration) have been addressed.

Now waiting the CosmJS PR to be in a tagged release.

@ethanfrey
Copy link
Contributor

I was looking to prepare this for a merge now, but I think this is still not in a CosmJS release.

Your commit is in cosmjs:main, but I don't see it in the v0.25.4 commit history.

I guess we need to wait til v0.26.0 and update this then?

@RiccardoM
Copy link
Contributor Author

I guess we need to wait til v0.26.0 and update this then?

I guess so, yeah 😢 Any ETA on this @webmaster128?

@webmaster128
Copy link
Member

If this is blocking, I'd rather backport the feature to CosmJS 0.25.x than rushing out 0.26.

@ethanfrey
Copy link
Contributor

If it can be backported easily, I can update ts-relayer to use it.
I don't know how urgent this is, just one of my pending tasks I wanted to clean up.

@RiccardoM are you using this code now? Or was it just a demo to test compatibility?

@RiccardoM
Copy link
Contributor Author

@RiccardoM are you using this code now? Or was it just a demo to test compatibility?

We will use this feature in the next couple of weeks most likely. We are ending our implementation inside Desmos, then we will perform an on-chain upgrade to release it. Targeting the first week of July, so it might be worth backporting this

@ethanfrey
Copy link
Contributor

Nice, I would love to see you using a proper tagged release of the code. Much less compatibility and maintenance headaches

@webmaster128
Copy link
Member

At cosmos/cosmjs#839 you find a backport of the change. The known limitation is that block height 1 cannot be decoded. The fix for that requires breaking changes. Please have a look and let me know if that would unblock you.

@webmaster128
Copy link
Member

CosmJS 0.25.5 should serve your needs now

@ethanfrey
Copy link
Contributor

@RiccardoM please run yarn fix:prettier to fix up the code, so the CI will run it.

If it passes, I can merge this. I will add release notes for 0.2.0 (changing supported versions) and cut a release with this.

@ethanfrey
Copy link
Contributor

Okay, failing test now.
This is cuz we use v0.41.1 simd in the tests.

Do you want to update https://github.com/confio/ts-relayer/blob/main/scripts/simapp/env#L4 to v0.42.4 ?

That should pass.

@RiccardoM
Copy link
Contributor Author

@ethanfrey I've updated the envs and not it passes. I've also opened a new PR (#197) that I think should be included in the next release as well since Band requires custom gas limits to be set to receive packets properly

@ethanfrey ethanfrey merged commit a4f0205 into confio:main Jun 24, 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.

BeginBlock and EndBlock events handling
4 participants