Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Toml #30

Merged
merged 14 commits into from
Apr 9, 2019
Merged

Toml #30

merged 14 commits into from
Apr 9, 2019

Conversation

martyall
Copy link
Contributor

@martyall martyall commented Apr 8, 2019

fixes #19

It doesn't fix this directly, but we now have the ability to write the toml file, or read from the plasma config from the environment. Using sed to parse the toml file for values, this means we can also read the toml file. I didn't want to write a custom parser for this.

@martyall
Copy link
Contributor Author

martyall commented Apr 9, 2019

UPDATE: I am now using the command to generate a .toml file and then mounting the volume for the plasma container to use. This means that when you bring up the docker network, you need to bring up only cliquebait first, then deploy the contracts, generate the config, then you can launch the plasma node. The README explains more

purs/src/Plasma/Config/TOML.purs Outdated Show resolved Hide resolved
purs/src/Plasma/Config/TOML.purs Show resolved Hide resolved
purs/src/Plasma/Config/TOML.purs Show resolved Hide resolved
purs/src/Plasma/Config/TOML.purs Show resolved Hide resolved
toTomlValue = show >>> withQuotes

instance tomlValueMaybe :: TomlValue a => TomlValue (Maybe a) where
toTomlValue = maybe (withQuotes mempty) toTomlValue
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think doing toTomlValue = maybe (toTomlValue "") toTomlValue is nicer as it's much clear what we are using in case we have Nothing value.

In general I think Maybe a shuold have no TomlValue this instance as the TOML itself is not allowing NULL value, when there is no value the key should just be omitted, and using "" for Nothing might make it hard to parse the file. But if we are planning to use this just for this particular case and it works then it's fine to leave this instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the interest of using this for this particular case, I'm not worried about the Maybe. If you look at what get's generated by plasmad init this is the behaviour we want.

purs/src/Plasma/Config/TOML.purs Show resolved Hide resolved
purs/src/Plasma/Config/TOML.purs Outdated Show resolved Hide resolved
getPlasmaContractAddress (FromChanterelleArtifactFile provider fp)
Just addr -> case mkHexString addr >>= mkAddress of
Nothing -> liftEffect $ throw ("Error parsing PLASMA_ADDRESS: " <> show addr)
Just addr' -> getPlasmaContractAddress (AddressLiteral addr')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing AddressLiteral to getPlasmaContractAddress is besically no op why not just do that log here and pure addr', and remove the AddressSource data type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will decide whether or not to do this after we have a more standard setup script. My hope is that we can somehow do a fresh contract deployment / cliquebait for a test suite, but I also want to be able to run it from an existing deployment.

I did it yesterday for a reason I can't recall right now, but will only change it if I'm sure I don't want it. It works for now

@kejace
Copy link
Member

kejace commented Apr 9, 2019

UPDATE: I am now using the command to generate a .toml file and then mounting the volume for the plasma container to use. This means that when you bring up the docker network, you need to bring up only cliquebait first, then deploy the contracts, generate the config, then you can launch the plasma node. The README explains more

Great, I'll have to update the docker image in order for it to not confuse things, but this should be really easy. We'll also have to change the .travis.yml file because it calls prepare-plasma.

@martyall
Copy link
Contributor Author

martyall commented Apr 9, 2019

@safareli I'm going to dismiss a lot of these comments regarding adding the quotes. I'm less interested in making sense of the toml related parts and more interested in generating the example at the top of this file, which would require putting quotes around everything including ints and bools. I don't want to deal with figuring out what is accepted by the plasma program right now

blinky3713 and others added 2 commits April 9, 2019 08:44
@kejace
Copy link
Member

kejace commented Apr 9, 2019

Passed CI

@kejace kejace merged commit e755130 into master Apr 9, 2019
@kejace kejace deleted the toml branch April 9, 2019 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generate the plasma.toml file from the test config
3 participants