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

Made before_send interceptor deterministic #590

Merged
merged 4 commits into from
Aug 4, 2023

Conversation

miladz68
Copy link
Contributor

@miladz68 miladz68 commented Aug 4, 2023

This change is Reviewable

@miladz68 miladz68 requested a review from a team as a code owner August 4, 2023 14:01
@miladz68 miladz68 requested review from dzmitryhil, vertex451, ysv and wojtek-coreum and removed request for a team August 4, 2023 14:01
wojtek-coreum
wojtek-coreum previously approved these changes Aug 4, 2023
Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @dzmitryhil, @vertex451, and @ysv)

ysv
ysv previously approved these changes Aug 4, 2023
Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @dzmitryhil and @vertex451)

dzmitryhil
dzmitryhil previously approved these changes Aug 4, 2023
Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @vertex451)

@miladz68 miladz68 dismissed stale reviews from dzmitryhil, ysv, and wojtek-coreum via 453487a August 4, 2023 15:26
Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @vertex451)

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @vertex451)

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed (commit messages unreviewed), 2 unresolved discussions (waiting on @miladz68 and @vertex451)


integration-tests/modules/wasm_test.go line 1657 at r2 (raw file):

		msgIssue := &assetfttypes.MsgIssue{
			Issuer:        admin.String(),
			Symbol:        "abc" + fmt.Sprint(i),

The tokens should start with a and z to let the udenom be in the middle.


integration-tests/modules/wasm_test.go line 1700 at r2 (raw file):

		executeMsg,
	)
	requireT.NoError(err)

You need to add one more call here, since that tx, will be accepted, but new tx after won't.

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed (commit messages unreviewed), 3 unresolved discussions (waiting on @miladz68 and @vertex451)


integration-tests/modules/wasm_test.go line 1679 at r2 (raw file):

	// send coin from the contract to test wallet
	withdrawPayload, err := json.Marshal(map[bankMethod]bankWithdrawRequest{

Add please the comment here, that we actually test the multiple coins send here, not the wasm. And add please TODO to remove that test once we impelent: https://app.clickup.com/t/86857vqra

Copy link
Contributor Author

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed (commit messages unreviewed), 3 unresolved discussions (waiting on @dzmitryhil and @vertex451)


integration-tests/modules/wasm_test.go line 1657 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

The tokens should start with a and z to let the udenom be in the middle.

I made it random


integration-tests/modules/wasm_test.go line 1679 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Add please the comment here, that we actually test the multiple coins send here, not the wasm. And add please TODO to remove that test once we impelent: https://app.clickup.com/t/86857vqra

Done.


integration-tests/modules/wasm_test.go line 1700 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

You need to add one more call here, since that tx, will be accepted, but new tx after won't.

but that was not my observation, the tx did not succeed.

Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dzmitryhil and @vertex451)

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @vertex451)

@miladz68 miladz68 merged commit dc41e55 into master Aug 4, 2023
@miladz68 miladz68 deleted the milad/make_before_send_deterministic branch August 4, 2023 16:32
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.

4 participants