Move bridge contact addresses to primary configuration file.

[c0gent]

Changes

• Remove contract addresses from Database and add to Node.
• Rename the config::load module to parse.
• Update tests and README.

Side Comments and Suggestions

• The Error type needs to be redesigned using the failure crate rather than error_chain. error_chain encourages the use of string errors, which are largely useless to library consumers. Precise types are needed for every error. String-only errors should be forbidden.
• Formatting needs improvement (tabs -> spaces, line width).
• Fields within Database, Config, and many other structs are declared pub. Leaving struct fields public is rarely a good idea, it's convenient at first but it can create situations where fields could possibly be accidentally mutated when they shouldn't be. This might not seem to be possible for many of the structs because they're generally contained in an App and therefore an Arc, making them immutable in that case, but the idiomatic solution is to create getter methods (and setter when necessary). Who knows what future library changes could accidentally expose a field to mutation in an unintended way. If fields do not need to be exposed publicly pub(crate) is an ok solution but can still leaves the door open for potential future bugs to creep in.
• The tests within integration-tests fail. This has nothing to do with the changes made here. I don't see an issue for this.

If reviewers feel the above suggestions warrant more attention, let me know and I will create issues for each (and happily tackle those issues).

Closes #101

[rstormsf]

@akolotov
@yrashk
please take a look

[akolotov]

the comment to c error messages added

[akolotov]

Does it make sense to raise ConfigError here the same as it is done in https://github.com/c0gent/poa-bridge/blob/45d643cb48adcd9d6beec3b9192017d791f18e99/bridge/src/config.rs#L130

[c0gent]

I'm not exactly sure I understand you. Are asking whether this error check is redundant or unnecessary?

If you're implying that the string error is a bad idea I agree. I'd be happy to simply use the ConfigError variant or create a new variant. As I mentioned, I badly want to completely revamp the error handling and remove all string errors and replace them with dedicated variants but I didn't want to include all of that in this pull request.

Let me know what you want me to do with that error for now.

[akolotov]

I read your note in the PR after adding my comment, sorry.
I understand what you mean by suggesting replacing all string errors to variants. And I would say that my comment is a suggestion to keep consistency before variants will be introduced: ConfigError already used in the line 130 to mark the error message as related to configuration so it make sense to use ConfigError in the line 164 as well.
What do you think?

[c0gent]

Agreed, I'll change it now.

[akolotov]

thanks

[DrPeterVanNostrand]

Yup, I deleted it immediately after posting it. I saw that it was an old piece of code, that just showed up green in the diff. You don't have to remove it.

[DrPeterVanNostrand]

Should this be an Option<Address> or could we just use an Address?

In the situation where we build with the deploy feature, it is required to be Some (deploy.rs panics if either the home or foreign contract address is None). In the case where we don't build with any features, we still require home and foreign contract addresses to be known to create web3 log filters for the home and foreign chains in deposity_relay.rs, withdraw_relay.rs, and withdraw_confirm.rs.

[c0gent]

Good question. One thing I considered was adding a #[cfg(not(feature = "deploy")) tag to the field and removing the Option. Doing so would require more tags everywhere the field was accessed though.

My understanding is (and please correct me if I'm wrong) that there isn't necessarily a 1:1 correspondence between Config and Database. Databases sometimes have their configurations loaded from stored strings, etc. I didn't see a clean solution for cases where a database configuration is loaded from an outside source because you'd then have to update the app.configuration with those values. For some time at least before the database config was parsed in, the app.config would need to have a None value. Alternatively, it would be possible to decouple the parts of the configuration specific to databases from app.configuration. It didn't seem like a great solution. I'm certainly open to any suggestions, there could easily be a good way to organize it that haven't thought of.

[DrPeterVanNostrand]

I think that I'm confused about when we will use the config object to retrieve the contract addresses and when we will use the database. Is having the home and foreign contract addresses in both objects redundant?

[c0gent]

The database configurations, from what I understand, may possibly be stored in a previously serialized format (via the Database::save/store method). I'm not sure exactly how they're used in practice but I imagine it's convenient to be able to back up a database configuration as a whole unit.

If its not necessary to store database configs containing contract addresses, and there is a guarantee to always have a good configuration loaded from the config.toml, I can remove that functionality entirely. In which case we could probably also remove the Option from the Note::contract_addresses as well.

[DrPeterVanNostrand]

Everything in the PR is really good so far.

from what I understand, may possibly be stored in a previously serialized format

This is a good point, but I would assume anyone who currently is using a db.toml file and is regularly updating their poa-bridge instance with master, will be able to change their contract address fields from their database file to their config file.

From the line of thinking in issue #101 that:

• A database is used for persistent storage of block numbers that have been
  checked for various contract events. The bridge has access to write to this
  file during runtime.

• The config file is read only, and should contain everything else.

We could probably do the following:

• We should remove home_contract_address and foreign_contract_address from the database structures in database.rs.

• parsed::Node (in config.rs) should have the field: contract_address: Option<Address>,

• Node (in config.rs) should have a field contract_address: Address.

• Node::from_load_struct() should contain something like the following:

let contract_address = match node.contract_address {
    Some(addr) => addr,
    None => {
        let msg =
            "Config file is missing contract address. Please define a \
            `contract_address` for both `[home]` and `[foreign]`.".into();
        
        return Err(ErrorKind::ConfigError(msg).into());
    },
};
...

let node = Node {
    ...
    contract_address,
    ...
};

and this block could be removed:

if cfg!(not(feature = "deploy")) && node.contract_address.is_none() {
    ...
}

• In desposit_relay.rs, withdraw_relay.rs, and withdraw_confirm.rs change any init.home/foreign_contract_address -> app.config.home/foreign.contract_address.

[c0gent]

Ok great, that clarifies things.

Forgive my ignorance but could you explain to me when deploy builds are used and what the usage under that configuration entails? I don't quite have a clear picture of the entire ecosystem and I haven't seen any documentation about it.

Also, do we need to worry about handling stored configurations from the prior (current) version of poa-bridge in an especially user-friendly way? Usually when I make a configuration file change to a program I leave all sorts of friendly error messages, etc. to ease the transition. How much of that is necessary here? Are the 'stored', serialized configuration files loaded in by the Deploy struct, for example, kept somewhere persistently or are they generated from scratch each time the bridge is started? Please point me to any material that might help me understand how the bridge fits in to its surrounding software elements.

I'll try out the changes you've suggested this afternoon or evening. Thanks a lot for your help :)

[yrashk]

@c0gent Nick, as for the deploy feature, it is on its way out as it is a remnant of the original version that used to do the deployment itself. It is currently only used by integration tests and this will removed with the transition to v2 of contracts (#103)

[c0gent]

I see. That simplifies things a bit. Thank you.

[c0gent]

I'll push my latest changes incorporating these ideas sometime tomorrow afternoon.

[c0gent]

Database has been cleaned back up per suggestions by @DrPeterVanNostrand. Let me know and I will squash the commits.