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

imp(ibc-testkit): remove field access of MockContext #1046

Merged
merged 5 commits into from
Jan 18, 2024

Conversation

rnbguy
Copy link
Collaborator

@rnbguy rnbguy commented Jan 18, 2024

Closes: #1043

Description

Removes ctx.events or context.events access from the tests.

This PR also moves .events and .logs fields under MockIbcStore to keep them under Arc<Mutex<MockIbcStore>>.


PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests.
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).
  • Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).

@rnbguy rnbguy self-assigned this Jan 18, 2024
Copy link
Member

@Farhad-Shabani Farhad-Shabani left a comment

Choose a reason for hiding this comment

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

Thanks @rnbguy. Just a quick note about this change:

My understanding so far, an IBC module usually should not autonomously store any events and logs. I know it is so in the basecoin-rs, but rationally, the storage access in IBC is designed to be either provable or private, serving the purpose of maintaining a record of IBC states as outlined in the specs.

Hosts typically maintain a sort of pool that collects dispatched events and logs from various modules and different parts of the system. Then the aggregated arrays would be included in the response of a transaction. Though, to achieve this, we should decouple storage from keys, so fields in IbcMockStore would act as mappers instead of having instant access to the store. Nevertheless, for now, we are fine with this change.

@Farhad-Shabani Farhad-Shabani changed the title refactor(ibc-testkit): remove field access of MockContext imp(ibc-testkit): remove field access of MockContext Jan 18, 2024
@rnbguy
Copy link
Collaborator Author

rnbguy commented Jan 18, 2024

Totally see your point. But my view is that, since we are working in the context of the IBC module, I see them as effects of the IBC module. So, to simplify things, I wanted to put everything that IBC affects - stores, logs, events; in one unified struct.

@rnbguy rnbguy merged commit ccef193 into main Jan 18, 2024
10 checks passed
@rnbguy rnbguy deleted the 1043-remove-field-access-of-mockcontext branch January 18, 2024 15:48
Farhad-Shabani pushed a commit that referenced this pull request Sep 9, 2024
* move events and logs inside MockIbcStore

* add getters for ibc events and logs

* refactor tests

* add doc strings

* add changelog
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.

imp(ibc-testkit): remove field access of MockContext
2 participants