#4394 Removed all HC Commitment code #4409

klercker · 2024-09-09T12:47:07Z

Removal of all HC Comittment code

functionality
method args
tests
dead code removal

This PR is supported by the Æternity Foundation

Squashed all branch commits [86b0ddc] More dead code removal and fixes to tests [0f3c630] reset incorrect commitment removal [4b04edf] Further code removal [2dd282a] Removed (most?) commitment code [c2286f6] Revert to master

hanssv

Looks good to me, some minor comments.

Also I suspect there is a bit too much removed in BTC/Doge HTTP clients - pinning will be creating/signing/posting transactions 🤔 Since we are supposed to support at least BTC we probably want to keep that?

apps/aecore/src/aec_parent_connector.erl

ThomasArts · 2024-09-09T13:27:27Z

apps/aecore/src/aec_parent_connector.erl

@@ -8,15 +8,6 @@

 -module(aec_parent_connector).

-%% Functionality:


You deleted all functionality comments, does that mean that the remaining code has no functionality at all?

yes, dead code(rs) society is here.

klercker · 2024-09-09T13:28:34Z

Yes, A bunch of code that is very likely usable in pinning was removed there. Unused methods were causing fails in dialyzer tests (at least that was what it looked like). I will dig in Git history and reinsert once we get to that point.

…

On Mon, Sep 9, 2024 at 3:26 PM Hans Svensson ***@***.***> wrote: ***@***.**** commented on this pull request. Looks good to me, some minor comments. Also I suspect there is a bit too much removed in BTC/Doge HTTP clients - pinning will be creating/signing/posting transactions 🤔 Since we are supposed to support at least BTC we probably want to keep that? ------------------------------ In apps/aecore/src/aec_parent_connector.erl <#4409 (comment)>: > %%%============================================================================= %%% Gen Server Callbacks %%%============================================================================= -spec init([any()]) -> {ok, #state{}}. -init([ParentConnMod, FetchInterval, ParentHosts, NetworkId, SignModule, HCPCPairs, Recipient, Fee, Amount]) -> - if is_integer(FetchInterval) -> - erlang:send_after(FetchInterval, self(), check_parent); - true -> ok - end, - CDetails = - #commitment_details{ parent_network_id = NetworkId, %% TODO: assert all nodes are having the same network id - sign_module = SignModule, - recipient = Recipient, - amount = Amount, - fee = Fee - }, +init([ParentConnMod, FetchInterval, ParentHosts, _NetworkId, _SignModule, HCPCPairs, _Recipient, _Fee, _Amount]) -> Are you expecting to use these arguments for pinning or could they be removed completely? ------------------------------ In apps/aecore/src/aec_parent_connector.erl <#4409 (comment)>: > + % if is_integer(FetchInterval) -> + % erlang:send_after(FetchInterval, self(), check_parent); + % true -> ok + % end, Remove? — Reply to this email directly, view it on GitHub <#4409 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIQDLIHJNIYX5FPP6B2S3DZVWOX3AVCNFSM6AAAAABN4PDECWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEOBZHA4TQMBSGQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

klercker · 2024-09-09T13:29:13Z

Exactly :P

…

On Mon, Sep 9, 2024 at 3:27 PM Thomas Arts ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In apps/aecore/src/aec_parent_connector.erl <#4409 (comment)>: > @@ -8,15 +8,6 @@ -module(aec_parent_connector). -%% Functionality: You deleted all functionality comments, does that mean that the remaining code has no functionality at all? — Reply to this email directly, view it on GitHub <#4409 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIQDLO7NK5RC27ZBTCIXZLZVWO5LAVCNFSM6AAAAABN4PDECWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEOBZHEYTQOBYHE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

ThomasArts · 2024-09-09T13:33:48Z

apps/aehttp/src/aehttpc_aeternity.erl

@@ -7,9 +7,6 @@
 -export([get_latest_block/2,


My feeling is that this whole module can be deleted if we don't care about commitments. What else did it offer that is not in the standard http client software??

For similarity to bitcoin and dodge module, we could keep it, of course. We still need to be able to see what epoch the parent chain is in

It's just a wrapper but since it is a wrapper that gets dynamically selected (ae, btc, doge) there needs to be one for aeternity as well. I kind-of like this way, so I think it should be kept.

hanssv · 2024-09-09T13:34:15Z

Yes, A bunch of code that is very likely usable in pinning was removed there. Unused methods were causing fails in dialyzer tests (at least that was what it looked like). I will dig in Git history and reinsert once we get to that point.

Either (temporarily) export the functions, or keep them commented out. I think the first is preferred.

ThomasArts · 2024-09-09T13:37:09Z

apps/aehttp/src/aehttpc_doge.erl

-    catch E:R:S ->
-        {error, {E, R, S}}
-    end;
+% getblock(NodeSpec, Seed, Hash, 2) ->


Should we not be able to get a block here?

It's not used anywhere, and is only called with (,,_,1) internally, but I kept it because it felt, I dunno, prudent(?)

@ThomasArts the problem with that particular method (head) is that it fails dialyzer:

The pattern <NodeSpec, Seed, Hash, 2> can never match the type <,,_,1>

That is why I edited it out What to you suggest I do with it? Expose it directly, write additional wrappers for get_latest_blocks?

ThomasArts · 2024-09-09T13:37:45Z

apps/aehttp/src/aehttpc_doge.erl

-    catch E:R ->
-        {error, {E, R}}
-    end.
+% -spec getrawtransaction(aehttpc:node_spec(), binary()) -> {ok, binary()} | {error, term()}.


Possibly useful as well when dealing with dodge as parent chain?

Again, I can reintroduce all the pruned code from these aehttpc modules (or whatever they are called) and export them. then these would make more sense. And not trigger build fails I suppose?

ThomasArts · 2024-09-09T13:38:40Z

apps/aehttp/test/aehttp_parent_connector_tests.erl

@@ -10,7 +10,8 @@
 -define(CHILD_CHAIN_NETWORK_ID, <<"hc_network_id">>).
 -define(SIGN_MODULE, aec_preset_keys).

-ae_sim_test_() ->
+% removed as test, should/could be removed altogether, same goes for btc version below.
+ae_sim_test_rem() ->


test_ as ending of a function name has a specific meaning. Cannot just at rem after it without changing that meaning. See eunit documentation for detail

I know, this leaves that particular test out of the suite. So here I could keep that code, in case it was actually testing something besides commitments. But perhaps it is not?

So, my question is if the two "_rem" methods (that are left in the code but not invoked as tests...) actually do anything but commitment testing, or of they can be removed altogether?

They do "test" that indeed one can talk to the parent BTC or parent Dodge. But My proposal is to just delete these two tests completely instead of renaming them _rem

You want to write very similar tests as soon as you add pinning, right?

- removed non-needed args in parent_connector:startlink - various cleanups

ThomasArts · 2024-09-10T08:17:01Z

apps/aecore/src/aec_parent_connector.erl

-%% - To BTC/Doge parent - signed by the external bitcoind/dogecoind hosted wallet
+%% Functionality
+%% Holds parent chain connection state and contains various utilities for 
+%% interacting withvthe PC. Wraps parent implementation specific stuff


Suggested change

%% interacting withvthe PC. Wraps parent implementation specific stuff

%% interacting with the PC. Wraps parent implementation specific stuff

- added comments to explain "disabled" test methods, left the methods for future ref.

klercker · 2024-09-10T08:55:07Z

So, these worked on my build locally. Intermittent? Re-run?

klercker added 2 commits September 9, 2024 14:16

Starting by commenting out code, no removal yet

ad55d62

hanssv reviewed Sep 9, 2024

View reviewed changes

apps/aecore/src/aec_parent_connector.erl Outdated Show resolved Hide resolved

apps/aecore/src/aec_parent_connector.erl Outdated Show resolved Hide resolved

ThomasArts reviewed Sep 9, 2024

View reviewed changes

klercker added 2 commits September 9, 2024 17:21

- Reverted back some util methods in aehttpc

d36ca23

- removed non-needed args in parent_connector:startlink - various cleanups

Util method fixes

da866bc

ThomasArts reviewed Sep 10, 2024

View reviewed changes

- Fix for consensus_hc invocation

eb0f6c9

- added comments to explain "disabled" test methods, left the methods for future ref.

hanssv approved these changes Sep 10, 2024

View reviewed changes

ThomasArts approved these changes Sep 10, 2024

View reviewed changes

klercker merged commit cc51f94 into master Sep 10, 2024
40 checks passed

klercker deleted the 4394-nocommitment branch September 10, 2024 12:03

dincho added the kind/improvement label Dec 5, 2024

happi added the area/hyperchains label Jan 20, 2025

		@@ -8,15 +8,6 @@

		-module(aec_parent_connector).

		%% Functionality:

	%% interacting withvthe PC. Wraps parent implementation specific stuff
	%% interacting with the PC. Wraps parent implementation specific stuff

#4394 Removed all HC Commitment code #4409

#4394 Removed all HC Commitment code #4409

Uh oh!

Conversation

klercker commented Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hanssv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ThomasArts Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klercker commented Sep 9, 2024 via email

Uh oh!

klercker commented Sep 9, 2024 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hanssv commented Sep 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klercker commented Sep 10, 2024

Uh oh!

Uh oh!

Uh oh!

klercker commented Sep 9, 2024 •

edited

Loading

ThomasArts Sep 9, 2024 •

edited

Loading