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

Use EthHash compatible type for subscription and filter IDs #9808

Merged
merged 5 commits into from
Dec 7, 2022

Conversation

iand
Copy link
Contributor

@iand iand commented Dec 7, 2022

Related Issues

Fixes filecoin-project/ref-fvm#1189 and filecoin-project/ref-fvm#1208

Fixes some itest breakage introduced around 23007b0

Proposed Changes

Uses 32 byte arrays for subscriiption and filter ids, populated from a 16 byte uuid which should be good enough randomness.

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

  • Commits have a clear commit message.
  • PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
    • area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
  • New features have usage guidelines and / or documentation updates in
  • Tests exist for new functionality or change in behavior
  • CI is green

@iand iand requested a review from a team as a code owner December 7, 2022 12:55
Copy link
Contributor

@arajasek arajasek left a comment

Choose a reason for hiding this comment

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

Would the following refactor be possible:

  • Move the new FilterID type to chain/types/event.go
  • Have the API use FilterID instead of EthFilterID
  • Drop EthFilterID entirely?

return FilterID{}, xerrors.Errorf("new uuid: %w", err)
}
id := FilterID{}
copy(id[:], rawid[:]) // uuid is 16 bytes
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
copy(id[:], rawid[:]) // uuid is 16 bytes
copy(id[:], rawid) // uuid is 16 bytes, the last 16 bytes are zeroed

@iand
Copy link
Contributor Author

iand commented Dec 7, 2022

  • Move the new FilterID type to chain/types/event.go
  • Have the API use FilterID instead of EthFilterID

Yes to these two

  • Drop EthFilterID entirely?

We need to keep this to preserve the Ethereum specific json marshalling

@iand
Copy link
Contributor Author

iand commented Dec 7, 2022

@arajasek PTAL

Copy link
Contributor

@arajasek arajasek left a comment

Choose a reason for hiding this comment

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

LGTM

@arajasek arajasek merged commit cfceafb into feat/nv18-events Dec 7, 2022
@arajasek arajasek deleted the issue/ref-fvm-1189 branch December 7, 2022 20:28
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