refactor(core-transaction-pool): segregate pools by wallets

[rainydio]

Summary

Each sender gets his own pool state and cannot interfere with others.

Requires #3536

Checklist

• Documentation
• Ready to be merged

[codecov]

Codecov Report

Merging #3499 into 3.0 will not change coverage by %.
The diff coverage is n/a.

@@           Coverage Diff           @@
##              3.0    #3499   +/-   ##
=======================================
  Coverage   48.50%   48.50%           
=======================================
  Files         548      548           
  Lines       13676    13676           
  Branches     1873     1873           
=======================================
  Hits         6634     6634           
  Misses       7013     7013           
  Partials       29       29           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e825bd9...e825bd9. Read the comment docs.

[faustbrian]

Will go over this on monday.

[faustbrian]

@rainydio merge conflicts

[faustbrian]

I left some initial (mostly cosmetic/semantic) remarks but there are a few more things in general.

Types

Always add types to variables, functions and return values. I know that TypeScript and an IDE can just show you all types but the code should reveal its intent, structure and types without needing an IDE to see what is what or having to jump around 5 files to see what a function is returning.

If someone is using vim, emacs or some barebones editor like sublime text they will need to dig through the code to see that method X from another package is returning an object of a specific type because the use of the code only was const var = class.method(); instead of const var: ObjectType = class.method();.

This might seem redundant at first glance but goes a long way in terms of clarity, self documenting code and documentation generation.

Naming

Variables

Avoid single character variable names as they won't communicate to a developer what is assigned to the variable and if you combine that with the lack of types it becomes even more of an issue. It should be apparent what a variable contains just by glancing it without having to look at other files, classes or functions.

Errors

Avoid ambiguous names like DuplicateError as they don't provide a whole lot of clarity when going through some code you rarely look at and make it harder to figure out what is trying to be achieved when seeing a bit of code like variable instance DuplicateError.

The first question that would pop to my mind when I see this code would be, what is duplicated? If there would be a check for an error named TransactionAlreadyInPoolError it would be blatantly obvious what is happening and why this error is being looked for.

---

I'll go over the PR again after the initially requested changes have been made but a great PR that was long overdue now that we have proper IoC thanks for TypeScript and Inversify.

[rainydio]

Always add types to variables, functions and return values.

Modern languages usually choose inferred types on local variables. Go and Rust were designed this way. C# added var keyword more than 10 years ago. Even conservative Java added var keyword couple of years ago too.

I know that TypeScript and an IDE can just show you all types... If someone is using vim, emacs or some barebones editor like sublime text they will need to dig through the code

This argument of IDE vs plain editor is ages old. I think in this particular case the shift occurred not due to IDEs getting better.

Explicit types hurt readability. They clutter code with information that is only needed if you try to edit it without IDE. For example documentation example snippets (any language) which by definition strike for highest readability use inferred local variable types.

Explicit types hurt code review. Around 5 to 10 years ago online code review tools got popular. The way people read code review diffs is very similar to documentation example snippets. The meaning is in variable and method names, and how they interact. It's difficult to review big chunks of code cluttered with information that's only needed to edit code without IDE.

I'm concerned that explicit local variable types are bad for code. Way more effort is needed to lay down convincing arguments than to add type information.

---

Regarding one character variable names, you are probably correct. I'm gonna fix that.

Regarding errors, you are probably correct. I used the names that are similar to type field that was already there (e.g. ERR_POOL_FULL translated to PoolFullError). This doesn't have to be that way or type field can be changed too. Next PR will focus on errors and processor and we can discuss this problem there.

[faustbrian]

Modern languages usually choose inferred types on local variables. Go and Rust were designed this way. C# added var keyword more than 10 years ago. Even conservative Java added var keyword couple of years ago too.

This argument of IDE vs plain editor is ages old. I think in this particular case the shift occurred not due to IDEs getting better.

The argument is ages old and still sticks around for a reason. When you jump around dozens of companies you'll meet as many people that don't use IDEs as do use IDEs because they prefer a more minimal setup without all the clutter and noise that modern IDEs add to your development environment.

Explicit types hurt readability. They clutter code with information that is only needed if you try to edit it without IDE. For example documentation example snippets (any language) which by definition strike for highest readability use inferred local variable types.

Explicit types hurt code review. Around 5 to 10 years ago online code review tools got popular. The way people read code review diffs is very similar to documentation example snippets. The meaning is in variable and method names, and how they interact. It's difficult to review big chunks of code cluttered with information that's only needed to edit code without IDE.

I'm concerned that explicit local variable types are bad for code. Way more effort is needed to lay down convincing arguments than to add type information.

Proper naming is obviously key but I would argue the exact opposite in terms of readability and code review problems. If I am required to use an IDE with plugins or dig through files unrelated directly to the changes of a PR to make out what type a variable has or what is returned by a function it is more distracting to me than having the code itself tell me about it.

Could argue that is personal preference but I would say that in general it makes it easier for new people to jump into the code as everything is right at your hand without having to dig deeper to see what class X from 5 dependencies down is returning because the variable doesn't tell me.

Regarding errors, you are probably correct. I used the names that are similar to type field that was already there (e.g. ERR_POOL_FULL translated to PoolFullError). This doesn't have to be that way or type field can be changed too. Next PR will focus on errors and processor and we can discuss this problem there.

If you will create another PR that reworks them right after this is merged it might be better to do them completely in a separate a PR instead of merging something that gets touched up again right after being merged.

[rainydio]

If you will create another PR that reworks them right after this is merged it might be better to do them completely in a separate a PR instead of merging something that gets touched up again right after being merged.

Merged before (similarly to #3500 and #3501). It's a bit difficult to cut only errors and processor, but let me try.

[faustbrian]

Looks good, will merge this once @air1one finished the 3.0 merge which he started yesterday.

[rainydio]

Because it's delayed I also updated log lines and created separate PR #3536 that implements transaction toString method.