Skip to content

Conversation

@yujingr
Copy link

@yujingr yujingr commented Jan 9, 2025

this allows for automated executable building later

@@ -0,0 +1,72 @@
name: Build Arbiter Signer
Copy link
Contributor

@mollkeith mollkeith Jan 10, 2025

Choose a reason for hiding this comment

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

Good job, but there are a few points that need adjustment:

  1. There will be multiple apps in the future, not just Arbiter. You can refer to the feature_web branch, which is still in development. So it's better to write a generic build check method instead of just building Arbiter.
  2. The go.yaml file under workflows path needs to be deleted to avoid duplication.
  3. Additionally, the workflow here will affect the build check in the README.md, so it needs to be modified and tested accordingly in the README

} `yaml:"arbiter"`
}

func getDefaultConfig() ConfigFile {
Copy link
Contributor

@mollkeith mollkeith Jan 10, 2025

Choose a reason for hiding this comment

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

It is not possible to enforce that config.yaml be placed in the current location:
GoFrame is designed to look for configuration files sequentially from the current directory and then from the manifest/config directory. Since a single repository can have multiple different config.yaml files, they need to be placed in their respective manifest directories. The official recommended location is always under the manifest/config directory. the config retrieval order of arbiter:

1. Arbiter_Signer/
2. Arbiter_Signer/config
3. Arbiter_Signer/manifest/config
4. Arbiter_Signer/app/arbiter/
5. Arbiter_Signer/app/arbiter/config
6. Arbiter_Signer/app/arbiter/manifest/config 

If you place a config.yaml file in the '.' directory, it will override the config.yaml under the manifest directory.

return createKeyFiles(cfg)
}

func createKeyFiles(cfg ConfigFile) error {
Copy link
Contributor

@mollkeith mollkeith Jan 10, 2025

Choose a reason for hiding this comment

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

Users should not be prompted to input during the running process, as the current deployment is automated through scripts or Docker. Any process that requires user input will disrupt the deployment. If it is indeed necessary, it should be changed to: 'If the key file exists, use the key file.'
Maybe this is not really necessary, as creating a default config.yaml will affect GoFrame's configuration retrieval method.

wg.Add(1)

// Initialize config file path for gf framework
g.Cfg().GetAdapter().(*gcfg.AdapterFile).SetPath(".")
Copy link
Contributor

Choose a reason for hiding this comment

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

remove it, useless code does not affect GoFrame's own configuration retrieval


# Arbiter
arbiter:
listener: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

good!

escArbiterAddress: "0x0262aB0ED65373cC855C34529fDdeAa0e686D913"
dataPath: "./app/arbiter/data"
keyFilePath: "./app/arbiter/data/keys/"
dataPath: "./data"
Copy link
Contributor

@mollkeith mollkeith Jan 10, 2025

Choose a reason for hiding this comment

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

No need to modify the default values here

  1. This config.yaml is just for convenience during local testing. You can modify the config.yaml example in README.md and adjust it as needed.
  2. Additionally, do not place the data directory under the '.' directory directly, as there may be multiple apps, and each app's data should be placed in a separate directory. If consolidation is necessary, you might consider renaming it to something like './data/arbiter' or similar

@mollkeith
Copy link
Contributor

It is recommended to split the changes into different pull requests for better management @yujingr

@yujingr
Copy link
Author

yujingr commented Jan 13, 2025

@mollkeith thanks for the comments and review. I should specify my intention here - to allow non-technical community members to be able to do a one-click setup. The code change prepares for the next step - automated github action that builds this into executables so that these non-technical members can quickly get the arbiter up and running. Therefore, the keys and config here are designed to be created during the setup. If I resolve the config file placement, do you think it would avoid conflicts down the road?

@yujingr
Copy link
Author

yujingr commented Jan 13, 2025

@mollkeith thanks for the comments and review. I should specify my intention here - to allow non-technical community members to be able to do a one-click setup. The code change prepares for the next step - automated github action that builds this into executables so that these non-technical members can quickly get the arbiter up and running. Therefore, the keys and config here are designed to be created during the setup. If I resolve the config file placement, do you think it would avoid conflicts down the road?

name: Build Arbiter Signer

on:
  push:
    branches: [ main ]
    tags:
      - 'v*' # Trigger on version tags
  pull_request:
    branches: [ main ]
  workflow_dispatch:

jobs:
  build:
    name: Build for ${{ matrix.os }}
    runs-on: ${{ matrix.os }}
    strategy:
      matrix:
        include:
          - os: ubuntu-latest
            output_name: arbiter-linux
            goarch: amd64
          - os: windows-latest
            output_name: arbiter-windows.exe
            goarch: amd64
          - os: macos-latest
            output_name: arbiter-macos-amd64
            goarch: amd64
          - os: macos-latest
            output_name: arbiter-macos-arm64
            goarch: arm64

    steps:
    - uses: actions/checkout@v4

    - name: Set up Go
      uses: actions/setup-go@v4
      with:
        go-version: '1.21'

    - name: Go Mod Tidy
      run: go mod tidy

    - name: Build
      env:
        GOOS: ${{ matrix.os == 'windows-latest' && 'windows' || matrix.os == 'ubuntu-latest' && 'linux' || 'darwin' }}
        GOARCH: ${{ matrix.goarch }}
      run: go build -o ${{ matrix.output_name }} ./app/arbiter

    - name: Upload Artifact
      uses: actions/upload-artifact@v4
      with:
        name: ${{ matrix.output_name }}
        path: ${{ matrix.output_name }}

  release:
    needs: build
    runs-on: ubuntu-latest
    if: startsWith(github.ref, 'refs/tags/v')
    steps:
      - name: Download all artifacts
        uses: actions/download-artifact@v4

      - name: Create Release
        uses: softprops/action-gh-release@v1
        with:
          files: |
            */arbiter-linux
            */arbiter-windows.exe
            */arbiter-macos-amd64
            */arbiter-macos-arm64
        env:
          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

@mollkeith
Copy link
Contributor

mollkeith commented Jan 13, 2025

@mollkeith thanks for the comments and review. I should specify my intention here - to allow non-technical community members to be able to do a one-click setup. The code change prepares for the next step - automated github action that builds this into executables so that these non-technical members can quickly get the arbiter up and running. Therefore, the keys and config here are designed to be created during the setup. If I resolve the config file placement, do you think it would avoid conflicts down the road?

@yujingr
Ok got it.
'build.yml' is good, but maybe conflicts with 'go.yml' and need to update build check of READEME .

We need to automatically deploy the arbiter, but the arbiter needs to have the configuration file and key file created before it starts. If the logic for creating the configuration file is included in the arbiter's code, then we would have to start the arbiter to generate the configuration file, and then modify the configuration file afterwards. That seems problematic.

@yujingr yujingr closed this Jan 15, 2025
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.

2 participants