Skip to content

op-node: Fixed Integers in channel frame header#3126

Merged
mergify[bot] merged 6 commits intodevelopfrom
jg/fixed_int
Aug 4, 2022
Merged

op-node: Fixed Integers in channel frame header#3126
mergify[bot] merged 6 commits intodevelopfrom
jg/fixed_int

Conversation

@trianglesphere
Copy link
Contributor

@trianglesphere trianglesphere commented Jul 28, 2022

Description
This switches the channel frame header over to using fixed integers. This avoids a circular
dependency when filling the serialized frame up to a maximum size. The circular dependency
is caused by the frame data length being a uvarint which can vary in size when serialized.

Metadata

  • Fixes ENG-2371
  • Fixes ENG-2537

@changeset-bot
Copy link

changeset-bot bot commented Jul 28, 2022

⚠️ No Changeset found

Latest commit: fd1b862

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@mergify
Copy link
Contributor

mergify bot commented Jul 28, 2022

This PR changes implementation code, but doesn't include a changeset. Did you forget to add one?

@trianglesphere trianglesphere force-pushed the jg/cleanup_channel_frame_serialization branch from 19f9215 to d0d9a61 Compare July 28, 2022 21:27
@trianglesphere trianglesphere force-pushed the jg/cleanup_channel_frame_serialization branch from d0d9a61 to 6f9926c Compare July 29, 2022 19:01
@mergify
Copy link
Contributor

mergify bot commented Jul 29, 2022

Hey @trianglesphere! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot added the conflict label Jul 29, 2022
@mergify mergify bot removed the conflict label Jul 29, 2022
@trianglesphere trianglesphere force-pushed the jg/cleanup_channel_frame_serialization branch from 6f9926c to 7539390 Compare August 1, 2022 16:59
Base automatically changed from jg/cleanup_channel_frame_serialization to develop August 2, 2022 20:08
@mergify
Copy link
Contributor

mergify bot commented Aug 2, 2022

Hey @trianglesphere! This PR has merge conflicts. Please fix them before continuing review.

@mergify
Copy link
Contributor

mergify bot commented Aug 3, 2022

Hey @trianglesphere! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot added the conflict label Aug 3, 2022
trianglesphere and others added 2 commits August 4, 2022 14:48
This mainly modifies the channel_frame, but has some knock on effects
as the width of some of the fields have been reduced. The channel
frame code is also changed more than I expected due to differences
in API of working with fixed int vs uvarints in go. Otherwise the
code reads very similarly with using Reader/Writer APIs.
Copy link
Contributor

@protolambda protolambda left a comment

Choose a reason for hiding this comment

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

looks good, but not approving just yet to avoid auto-merge, one nit style fix first.

@mergify
Copy link
Contributor

mergify bot commented Aug 4, 2022

This PR has been added to the merge queue, and will be merged soon.

@mergify mergify bot merged commit 04a39a4 into develop Aug 4, 2022
@mergify mergify bot deleted the jg/fixed_int branch August 4, 2022 20:31
@mergify
Copy link
Contributor

mergify bot commented Aug 4, 2022

This PR is next in line to be merged, and will be merged as soon as checks pass.

@mergify mergify bot removed the on-merge-train label Aug 4, 2022
This was referenced Aug 8, 2022
roninjin10 pushed a commit that referenced this pull request Aug 26, 2022
* goals for fixed int

* op-node: Fixed integer sizes in the channel header

This mainly modifies the channel_frame, but has some knock on effects
as the width of some of the fields have been reduced. The channel
frame code is also changed more than I expected due to differences
in API of working with fixed int vs uvarints in go. Otherwise the
code reads very similarly with using Reader/Writer APIs.

* op-node: fix frame unmarshal func to return correct err

* specs: update frame format specs

* Update op-node/rollup/derive/channel_bank_test.go

Co-authored-by: protolambda <proto@protolambda.com>
Co-authored-by: Matthew Slipper <me@matthewslipper.com>
maurelian pushed a commit that referenced this pull request Sep 15, 2022
* goals for fixed int

* op-node: Fixed integer sizes in the channel header

This mainly modifies the channel_frame, but has some knock on effects
as the width of some of the fields have been reduced. The channel
frame code is also changed more than I expected due to differences
in API of working with fixed int vs uvarints in go. Otherwise the
code reads very similarly with using Reader/Writer APIs.

* op-node: fix frame unmarshal func to return correct err

* specs: update frame format specs

* Update op-node/rollup/derive/channel_bank_test.go

Co-authored-by: protolambda <proto@protolambda.com>
Co-authored-by: Matthew Slipper <me@matthewslipper.com>
sam-goldman pushed a commit that referenced this pull request Sep 15, 2022
* goals for fixed int

* op-node: Fixed integer sizes in the channel header

This mainly modifies the channel_frame, but has some knock on effects
as the width of some of the fields have been reduced. The channel
frame code is also changed more than I expected due to differences
in API of working with fixed int vs uvarints in go. Otherwise the
code reads very similarly with using Reader/Writer APIs.

* op-node: fix frame unmarshal func to return correct err

* specs: update frame format specs

* Update op-node/rollup/derive/channel_bank_test.go

Co-authored-by: protolambda <proto@protolambda.com>
Co-authored-by: Matthew Slipper <me@matthewslipper.com>
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