[Utility] Foundational bugs, tests, code cleanup and improvements (1 / 2)

[Olshansk]

Description

This is the first of several changes necessary to refactor and improve the utility module to enable implementation of future, more complex, interfaces.

Issue

Fixes #475

Type of change

Please mark the relevant option(s):

• New feature, functionality or library
• Bug fix
• Code health or cleanup
• Major breaking change
• Documentation
• Other

List of changes

Changes

Utility

• Add a Validatable type for basic validation
• Split business logic specific to certain actors (e.g. validator reward, app relays, message handling) into separate files
• Reduce the scope of functions and types that shouldn’t be exposed
• Upgraded the actors tests - a lot went here to help with understanding what’s going on but it’s still just a start
• Remove the Context struct; unnecessary abstraction
• Added comments and guidance on message, transaction and signature validation
• Added ITransaction, an interface for the Transaction protocol to help capture the functionality it adds to the core type

Code Confusion

• Remove IUnstakingActor and use UnstakingActor directly. Guideline for removing future unnecessary types (e.g. TxResult)
• Delineate between unstaking & unbonding in a few places throughout the codebase

Bugs

• Avoid unstaking all actors when intending to UnstakeMsg paused actors only (major bug in persistence sql query)
• tx.Equals was comparing the same transaction against itself (major bug)
• Staking status enums in utility did not reflect the same protocol as in persistence (needs to be consolidated)

Code optimization

• Avoid redundant byte <-> string conversions in several places
• Avoid redundant bigInt <-> string conversions in several places

Code health

• Add comments explaining the use/responsibility of various types
• Consolidate BigInt/String converters used in other parts of the codebase
• Consolidate some types used everywhere (e.g. actorTypes)
• Reduce the code footprint of the codec package & add some TODOs
• Removed unused double sign code (moved to [Consensus] Penalize validators for double signing #432 for reference)

Files focused on

utility

• ├── account.go
• ├── account_test.go
• ├── actor.go
• ├── actor_test.go
• ├── application.go
• ├── application_test.go
• ├── block.go
• ├── block_test.go
• ├── context.go
• ├── doc
• │   ├── CHANGELOG.md
• │   ├── PROTOCOL_RELAY.md
• │   ├── PROTOCOL_SESSION.md
• │   └── README.md
• ├── gov.go
• ├── gov_test.go
• ├── message_handler.go
• ├── message_test.go
• ├── module.go
• ├── module_test.go
• ├── service
• │   └── service.go
• ├── session.go
• ├── transaction.go
• ├── transaction_test.go
• ├── types
• │   ├── constants.go
• │   ├── error.go
• │   ├── gov.go
• │   ├── message.go
• │   ├── message_staking.go
• │   ├── message_test.go
• │   ├── proto
• │   │   ├── message.proto
• │   │   ├── stake_status.proto
• │   │   ├── transaction.proto
• │   │   ├── tx_result.proto
• │   │   └── utility_messages.proto
• │   ├── relay_chain.go
• │   ├── relay_chain_test.go
• │   ├── signature.go
• │   ├── transaction.go
• │   ├── transaction_test.go
• │   ├── tx_fifo_mempool.go
• │   ├── tx_fifo_mempool_test.go
• │   ├── tx_result.go
• │   └── validatable.go
• └── validator.go

Testing

• make develop_test
• LocalNet w/ all of the steps outlined in the README

Required Checklist

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have tested my changes using the available tooling
• I have updated the corresponding CHANGELOG

If Applicable Checklist

• I have updated the corresponding README(s); local and/or global
• I have added tests that prove my fix is effective or that my feature works
• I have added, or updated, mermaid.js diagrams in the corresponding README(s)
• I have added, or updated, documentation and mermaid.js diagrams in shared/docs/* if I updated shared/*README(s)

[Olshansk]

@deblasis I'm still thinking through a couple of your comments and will follow up if I have a better idea. In the meantime, left some responses and addressed some of the changes.

[Olshansk]

I added it, but then actually did not end up using it as intended.

The idea was to show that some of our interfaces add functionality to serializable types based on the codec we're using.

	_ codec.ICodec = &Transaction{}

I'm removing it for now (since it is extra code that doesn't provide much benefit), but we should discuss that this is a very implicit design pattern others might not understand.

Adding the following in codec.go in the meantime:

// DOCUMENT: Some of the types through the code based (e.g. Transaction) are defined via serializable
// files (e.g. .proto files) and add functionality defined in go on top of it (e.g. ITransaction). This
// a very implicit design pattern that may be non-obvious to new developers.

[Olshansk]

After doing the above and running tests, I got the following errors:

utility/types/message.go:179:20: msg.ProtoReflect undefined (type Message has no field or method ProtoReflect)
utility/types/message.go:183:38: cannot use msg (variable of type Message) as type protoreflect.ProtoMessage in argument to codec.GetCodec().Marshal:

For now, I've renamed ICodec to CodecType so I can embed it in message but going to keep thinking.

[Olshansk]

@deblasis PTAL (please take another look).

Left some replies in the resolved comments as well as kept a few ones open. Will also update the chanelogs.

[deblasis]

@Olshansk I guess the pending convos are:

• [Utility] Foundational bugs, tests, code cleanup and improvements (1 / 2) #503 (comment)
• [Utility] Foundational bugs, tests, code cleanup and improvements (1 / 2) #503 (comment)
• [Utility] Foundational bugs, tests, code cleanup and improvements (1 / 2) #503 (comment)

They are mostly questions and nits. I feel confident to just approve at this point. The main points have been already addressed I reckon