Skip to content

Conversation

@holiman
Copy link
Owner

@holiman holiman commented May 21, 2024

This fixes the append vs overwrite part of #170 .

However, this is technically a breaking change. On the other hand, the way it now works is incompatible with the fastssz encoding library. And since nobody has raised this earlier, I assume nobody has really used it in production.

So, should be fine to merge, on that reasoning.

(?)

@codecov
Copy link

codecov bot commented May 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (70cbe2b) to head (e685369).
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #171   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines         1632      1638    +6     
=========================================
+ Hits          1632      1638    +6     

@jshufro
Copy link

jshufro commented May 21, 2024

(?)

I'd recommend going through the paces with deprecation instead, or at the very least keeping around a function that preserves the old behavior (under a new symbol).

@holiman
Copy link
Owner Author

holiman commented May 27, 2024

Deprecating MarshalSSZTo doesn't really work -- it's an interface method, so I think calls to to it are via not direct, and the deprecation won't be noticed by tooling.

Possibly, I could remove it, and add it back a few releases later. At least then the breakage would be obvious. And that's perhaps better than what this PR does, which breaks things in a way which will only show in runtime, not compile-time.

@jshufro
Copy link

jshufro commented May 28, 2024

calls to to it are via not direct

I can't imagine anyone is calling it any way except direct.

sszgen/fastssz do not really support external interfaces. The -include flag for sszgen expects a local package only. I had to do all this: https://github.com/jshufro/smartnode/blob/e55d271c981833425987fb84d20d03e6177a7276/shared/services/rewards/ssz_types/big/uint256.go

@holiman
Copy link
Owner Author

holiman commented May 29, 2024

type Uint256 struct {
	*big.Int
}

Hm, why can't you use uint256.Int directly, or, alternatively, alias it?

type Uint256 uint256.Int`

But I agree, perhaps best to just deprecate MarshalSSZTo in this PR, and create MarshalSSZAppend as a new method which does the append.

@holiman
Copy link
Owner Author

holiman commented Jun 11, 2024

Possibly, I could remove it, and add it back a few releases later. At least then the breakage would be obvious.

I have now changed this PR to remove MarshalSSZTo, to force users to choose either MarshalSSZAppend or MarshalSSZInto. The former is the future MarshalSSZTo, the latter is already deprecated.

@jshufro SGTY?

@holiman holiman changed the title conversion: fix append instead of overwrite in MarshalSSZTo. conversion: replace MarshalSSZTo with MarshalSSZAppend and MarshalSSZInto Jun 12, 2024
@holiman holiman merged commit 75a5209 into master Jun 12, 2024
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.

3 participants