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

feat(ics20): implement on receive packet #20

Merged
merged 11 commits into from
Aug 7, 2024
Merged

Conversation

gjermundgaraba
Copy link
Contributor

No description provided.

@@ -40,6 +42,27 @@ func populateQueryReqToPath(ctx context.Context, chain *cosmos.CosmosChain) erro
return nil
}

func ABCIQuery(ctx context.Context, chain *cosmos.CosmosChain, req *abci.RequestQuery) (*abci.ResponseQuery, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this one as a utility function, mostly for debugging to make sure commitments were really in the store as expected.

The reason I could not reuse the existing generic function was because the method it got from the request was wrong.

Copy link
Member

Choose a reason for hiding this comment

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

You could use GRPCQuery function below I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried, but the method/path came out wrong, for some reason. 🤷

e2e/interchaintestv8/ibc_eureka_test.go Outdated Show resolved Hide resolved
script/E2ETestDeploy.s.sol Outdated Show resolved Hide resolved
src/ICS26Router.sol Outdated Show resolved Hide resolved
src/ICS26Router.sol Outdated Show resolved Hide resolved
"{\"amount\":\"",
Strings.toString(amount),
"\",\"denom\":\"",
"{\"denom\":\"",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These were changed to match the order they come in from ibc-go. This is a limitation of the ICS20Lib: it only supports JSON in a specific order. Parsing the JSON in any order is possible, but might be more gas-intensive. Should we create an issue for that? There is a TODO below on this.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, let's create an issue. And please link the issues to the todo items (as numbers) if there is an associated issue.

@gjermundgaraba
Copy link
Contributor Author

@srdtrk, I had already implemented the seconds/nanoseconds checker, so I left it in here. But if you would prefer to remove it, we can revert that and change to only use nanoseconds that are divisible by 1,000,000,000 in the e2e test.

Copy link
Member

@srdtrk srdtrk left a comment

Choose a reason for hiding this comment

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

Amazing work as always

src/ICS20Transfer.sol Outdated Show resolved Hide resolved
src/ICS26Router.sol Outdated Show resolved Hide resolved
src/ICS26Router.sol Outdated Show resolved Hide resolved
script/E2ETestDeploy.s.sol Outdated Show resolved Hide resolved
e2e/interchaintestv8/ibc_eureka_test.go Outdated Show resolved Hide resolved
@@ -40,6 +42,27 @@ func populateQueryReqToPath(ctx context.Context, chain *cosmos.CosmosChain) erro
return nil
}

func ABCIQuery(ctx context.Context, chain *cosmos.CosmosChain, req *abci.RequestQuery) (*abci.ResponseQuery, error) {
Copy link
Member

Choose a reason for hiding this comment

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

You could use GRPCQuery function below I think

Copy link
Member

@srdtrk srdtrk left a comment

Choose a reason for hiding this comment

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

I'm approving after the pushes I've made. Great work. I'll let you respond to my review before merging this :)

@srdtrk srdtrk merged commit 298254c into main Aug 7, 2024
7 checks passed
@srdtrk srdtrk deleted the gjermund/ics20-on-recv-packet branch August 7, 2024 07:14
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.

2 participants