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

Eip778 #3701

Merged
merged 5 commits into from
Dec 19, 2021
Merged

Eip778 #3701

merged 5 commits into from
Dec 19, 2021

Conversation

tkstanczak
Copy link
Member

Resolves #2618

Changes:

Implements EIP-778

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Other (please describe):

Testing

Requires testing

  • Yes
  • No

In case you checked yes, did you write tests??

  • Yes
  • No

Copy link
Contributor

@dceleda dceleda left a comment

Choose a reason for hiding this comment

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

  • Maximum size of an encoded node record is 300 bytes.
    Should we enforce it (validate) or we assume that it's not going to happen with the current implementation?
  • I think it would be good if we start adding descriptions to all class we implement.

protected abstract int GetRlpLengthOfValue();

/// <summary>
/// Gets the key as a string to be added in the
Copy link
Contributor

Choose a reason for hiding this comment

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

Not finished comment?

public class NodeRecord
{
private SortedDictionary<string, EnrContentEntry> Entries { get; } = new();

Copy link
Contributor

@dceleda dceleda Dec 18, 2021

Choose a reason for hiding this comment

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

Property desc?
The sequence number, a 64-bit unsigned integer. Nodes should increase the number whenever the record changes and republish the record.

{
private SortedDictionary<string, EnrContentEntry> Entries { get; } = new();

public int Sequence { get; set; }
Copy link
Contributor

@dceleda dceleda Dec 18, 2021

Choose a reason for hiding this comment

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

Maybe worth renaming to Seq ? Or maybe EnrSequence ?

@dceleda dceleda self-requested a review December 19, 2021 10:50
@tkstanczak tkstanczak merged commit f56760e into master Dec 19, 2021
@tkstanczak tkstanczak deleted the eip778 branch December 19, 2021 10:50
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.

Implement EIP-778
2 participants