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

Add etna-time configs #463

Merged
merged 16 commits into from
Aug 30, 2024
Merged

Add etna-time configs #463

merged 16 commits into from
Aug 30, 2024

Conversation

iansuvak
Copy link
Contributor

Why this should be merged

This will allow for merging of ACP-118 changes and toggling them based on the configured timestamp of the Etna upgrade

How this works

  • New config option uses Viper as before but uses it's GetTime() method directly instead of relying on mapstructure tags since time.Time isn't directly handled by it.
  • Fixes up some documentation in the sample workflow section.

How this was tested

  • New simple E2E test was written and confirmed that both pre and post-etna cases are functioning
  • Ran a manual build of signature-aggregator against Fuji testnet with the etna-time configuration set in the past, unset and set in the future. As expected, only future and unset cases returned a valid signature.
  • Ran a manual build of signature-aggregator on Use ACP-118 interface #383 branch and confirmed that doesn't work either as expected.

How is this documented

@@ -67,6 +68,9 @@ type Config struct {
DeciderURL string `mapstructure:"decider-url" json:"decider-url"`
SignatureCacheSize uint64 `mapstructure:"signature-cache-size" json:"signature-cache-size"`

// mapstructure doesn't handle time.Time out of the box so handle it manually
EtnaTime time.Time `json:"etna-time"`
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 would have liked to do omitempty so that tests don't write the field out when marshaling JSON here but zero time value still gets written out unless the field type is *time.Time which seemed messier.

The value that does get written out is correctly parsed by viper to be zero time value. As a result tests run the pre-Etna case unless otherwise specified.

@iansuvak iansuvak mentioned this pull request Aug 28, 2024
Copy link
Contributor

@geoff-vball geoff-vball left a comment

Choose a reason for hiding this comment

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

One comment but LGTM 👍

}
}

func (s *SignatureAggregator) unmarshalResponse(responseBytes []byte) (blsSignatureBuf, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we leave TODOs and create a ticket for when this logic is okay to clean up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added TODO and entered: #466

dependabot bot and others added 10 commits August 29, 2024 12:37
Bumps [github.com/onsi/ginkgo/v2](https://github.com/onsi/ginkgo) from 2.20.1 to 2.20.2.
- [Release notes](https://github.com/onsi/ginkgo/releases)
- [Changelog](https://github.com/onsi/ginkgo/blob/master/CHANGELOG.md)
- [Commits](onsi/ginkgo@v2.20.1...v2.20.2)

---
updated-dependencies:
- dependency-name: github.com/onsi/ginkgo/v2
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
…m/onsi/ginkgo/v2-2.20.2

Bump github.com/onsi/ginkgo/v2 from 2.20.1 to 2.20.2
Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.65.0 to 1.66.0.
- [Release notes](https://github.com/grpc/grpc-go/releases)
- [Commits](grpc/grpc-go@v1.65.0...v1.66.0)

---
updated-dependencies:
- dependency-name: google.golang.org/grpc
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [github.com/onsi/gomega](https://github.com/onsi/gomega) from 1.34.1 to 1.34.2.
- [Release notes](https://github.com/onsi/gomega/releases)
- [Changelog](https://github.com/onsi/gomega/blob/master/CHANGELOG.md)
- [Commits](onsi/gomega@v1.34.1...v1.34.2)

---
updated-dependencies:
- dependency-name: github.com/onsi/gomega
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
…m/onsi/gomega-1.34.2

Bump github.com/onsi/gomega from 1.34.1 to 1.34.2
…lang.org/grpc-1.66.0

Bump google.golang.org/grpc from 1.65.0 to 1.66.0
@iansuvak iansuvak merged commit 0648123 into signature-aggregation-api-acp-118 Aug 30, 2024
7 checks passed
@iansuvak iansuvak deleted the etna-time branch August 30, 2024 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants