Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: events: version the MessageReceipt structure. #9636

Merged
merged 7 commits into from
Nov 15, 2022

Conversation

raulk
Copy link
Member

@raulk raulk commented Nov 14, 2022

This is implemented as an internal version discriminator. An alternative would've been to turn MessageReceipt into an interface with two backing structs for v0 and v1. But that approach would've broken all usage sites.

CBOR serde for MessageReceipt is no longer automatically generated. We use custom logic to deal with versioning in both directions. This logic uses the cborgen serde code as a basis.

Related Issues

Proposed Changes

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

@raulk raulk requested a review from a team as a code owner November 14, 2022 12:35
@raulk raulk force-pushed the raulk/events-messagereceipt branch from 3954733 to 5be87c0 Compare November 14, 2022 12:36
This is implemented as an internal version discriminator. An alternative
would've been to turn MessageReceipt into an interface with two backing
structs for v0 and v1. But that approach would've broken all usage sites.

CBOR serde for MessageReceipt is no longer automatically generated. We
use custom logic to deal with versioning in both directions. This logic
uses the cborgen serde code as a basis.
@raulk raulk force-pushed the raulk/events-messagereceipt branch from 5be87c0 to 8333b80 Compare November 14, 2022 12:37
Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

custom cbor looks incorrect and there is the issue of codecs in other languages, rust at minimum.

return err
}
} else {
if err := cw.WriteMajorTypeHeader(cbg.MajNegativeInt, uint64(-mr.ExitCode-1)); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

woah, we need to be able to decode this in other languages too you know...

Copy link
Member Author

Choose a reason for hiding this comment

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

This was generated by cborgen -- what is the concrete problem you see?

@vyzo
Copy link
Contributor

vyzo commented Nov 14, 2022

Cant we do something similar to the state tree?

@raulk raulk requested a review from vyzo November 15, 2022 11:24
Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

lools ok and we have test for the serializer, so green tick it is!

Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

You could avoid the pointer wrapping and make the version specific serde free standing functions, but up to you.

@vyzo
Copy link
Contributor

vyzo commented Nov 15, 2022

Note test failure.

@raulk
Copy link
Member Author

raulk commented Nov 15, 2022

You could avoid the pointer wrapping and make the version specific serde free standing functions, but up to you.

Not worth it. I expect that to be optimized away by newtype compiler optimizations anyway.

@raulk raulk merged commit c75d0c4 into feat/nv18-events Nov 15, 2022
@raulk raulk deleted the raulk/events-messagereceipt branch November 15, 2022 12:00
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants