ci: add elvis linter

[hlolli]

No description provided.

[JamesPiechota]

Why do we remove spaces here, but not here

[hlolli]

that is a good question, I believe the complaint was just about the parentheses and I did more than was needed to satisfy this lint rule

[JamesPiechota]

Is this an elvis change? Personally I prefer to keep newlines consistent within an operator. For this operator we've made the choice to put each list element on its own line, so I would prefer to also put the | Props on its own line to be consistent. I've been bitten before by inconsistent new lines where only one element is put on the same line as the others and it gets missed when ssomeone is scannign the code because they expect all elements to be on new lines.

[hlolli]

the default rule says that there can't be space on the left side of an operator, this has the effect that we have

ValA +
ValB +
ValC.

and not

ValA
+ ValB
+ ValC

personally I'm not trained to read code like thie

{
  a: 1
  ,b: 2
  ,c: 3
 }

[JamesPiechota]

I'm fine with

ValA +
ValB +
ValC.

or

ValA
+ ValB
+ ValC

and think in general commas should be treated differently from math operators for the reason you mention (commas generally bind to the left value, but it's less clear with binary math operators which value they more logically bind to - think of grammar school long addition, in the US we're taught to put the + at the beginning of every line)

But that said, I retract my original point on the | Elements and defer to the elvis rule

[JamesPiechota]

I believe this is a slight change in functionality. Previously Exception:Reason was sent, now {Exception, Reason} is sent. I'm not familiar with the code enough to know if it will break anything, given that I'd revert to the old behavior (unless you feel good that it won't break)

[hlolli]

good point, I don't know what the old catch did when it hit, I don't have a good feeling myself, to be fair I asked chatgpt to keep the behaviour the same. But I also wouldn't have a good feeling with making the catch case a no-op. Happy for suggestions.

The receiver is in ar_http_req.erl I think

read_complete_body(Req, #{ acc := Acc, counter := C } = Opts) ->
	{MoreOrOk, Data, ReadReq} = cowboy_req:read_body(Req),
	DataSize = byte_size(Data),
	prometheus_counter:inc(
		http_server_accepted_bytes_total,
		[ar_prometheus_cowboy_labels:label_value(route, #{ req => Req })],
		DataSize
	),
	read_complete_body(MoreOrOk, Opts#{ acc := [Acc | Data],  counter := C + DataSize }, ReadReq).

read_complete_body(_, #{ counter := C, size_limit := SizeLimit }, _) when C > SizeLimit ->
	{error, body_size_too_large};
read_complete_body(more, Data, Req) ->
	read_complete_body(Req, Data);
read_complete_body(ok, #{ acc := Acc, start_time := StartTime }, Req) ->
	Body = iolist_to_binary(Acc),
	BodyReadTime = erlang:monotonic_time() - StartTime,
	{ok, Body, with_body_req_fields(Req, Body, BodyReadTime)}.

[hlolli]

maybe @vird or @ldmberman could chip in here, what is returned in this loop code when an error is caught? Are we safe to just return {error, Reason} or should we return nothing?

[JamesPiechota]

Ditto above

[JamesPiechota]

ditto

[hlolli]

fixed

[JamesPiechota]

Suggested change

	{{_Utility_, TXID, _Status} = _, Iterator2} = gb_sets:next(Iterator),
	{{_Utility_, TXID, _Status} = _Element, Iterator2} = gb_sets:next(Iterator),

[JamesPiechota]

looks like we lost the {error, timeout} return

[hlolli]

I'll add it back, but it seems this can be any error, but I revert

[hlolli]

maybe take a look at the file now if it makes sense, I return error, timeout from the catch clause

[JamesPiechota]

I think the issue is the formatting of returns. Previously only when we caught {'EXIT', {timeout, {gen_server, call, [ar_arql_db, _]}}} did we return {error, timeout} - now any error will trigger an {error, timeout}. I'm not sure if it will make a difference, but I'm a little wary of functionality changes in a large formatting PR.

If there aren't any callers of get_tx_confirmation_data that handle {error, timeout}, then it's fine and we can go with your earlier iteration of {error, Exception} (or whatever)

but if the callers of get_tx_confirmation_data handle {error, timeout} or handle {error, SOMETHING}, then this diff might break those callers handling logic.

[hlolli]

I don't want any functional change, I'm aware that enforcing single case statement may be mutually exclusive with that goal. But let's look at this:

> git grep get_tx_confirmation_data                                           
apps/arweave/src/ar_header_sync.erl:250:        case ar_storage:get_tx_confirmation_data(TXID) of
apps/arweave/src/ar_http_iface_middleware.erl:1543:                                     case ar_storage:get_tx_confirmation_data(TXID) of
apps/arweave/src/ar_storage.erl:10:             read_tx/1, read_tx_data/1, update_confirmation_index/1, get_tx_confirmation_data/1,
apps/arweave/src/ar_storage.erl:301:get_tx_confirmation_data(TXID) ->
apps/arweave/test/ar_tx_blacklist_tests.erl:324:                        ?assertMatch({ok, {_, _}}, ar_storage:get_tx_confirmation_data(TXID))
apps/arweave/test/ar_tx_blacklist_tests.erl:351:                        ?assertMatch({ok, {_, _}}, ar_storage:get_tx_confirmation_data(TXID)

Excluding tests, we have callers in ar_http_iface_middleware.erl and ar_header_sync.erl

Looking at ar_http_iface_middleware.erl
it pattern matches on

{ok, {Height, BH}} ->
.... elided ...
						not_found ->
							{404, #{}, <<"Not Found.">>, Req};
						{error, not_found} ->
							{404, #{}, <<"Not Found.">>, Req};
						{error, timeout} ->
							{503, #{}, <<"ArQL unavailable.">>, Req}

and ar_header_sync.erl

which pattern matches on

		{ok, {Height, _BH}} ->
.....
		not_found ->
			ar_events:send(tx, {ready_for_unblacklisting, TXID}),
			{noreply, State};
		{error, Reason} ->
			?LOG_WARNING([{event, failed_to_read_tx_confirmation_index},
					{error, io_lib:format("~p", [Reason])}]),
			{noreply, State}

the former one only matches timeout errors, while the latter one matches any error. So any other error, Reason that isn't timeout would not fly with ar_http_iface_middleware but would be ok in ar_header_sync.

I can write up a ticket to make it consistent and allow any error reason to be sent and received, because surely timeout is only one of meany reasons.

[JamesPiechota]

Given what the callers are doing, and what the behavior was before, it looks like what you have now is good. The only change in functionality is now it will return {error, timeout} for some scenarios where it crashed outright before. I think that change in functionality is okay. (assuming I understand the whole {'EXIT', {... match which I'm not sure i do)