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

Use logic from commons #264

Merged
merged 6 commits into from
Apr 25, 2024
Merged

Use logic from commons #264

merged 6 commits into from
Apr 25, 2024

Conversation

Siegrift
Copy link
Collaborator

Closes #250

Rationale

While implementing the issue I realised there is some more code that should be reused from commons. Each commit focuses on a particular feature implemented in commons.

@Siegrift Siegrift requested a review from bdrhn9 April 23, 2024 09:38
@Siegrift Siegrift self-assigned this Apr 23, 2024
Copy link
Contributor

@bdrhn9 bdrhn9 left a comment

Choose a reason for hiding this comment

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

Minor suggestions other than LGTM.

src/state/state.ts Outdated Show resolved Hide resolved
signedDatas: Record<BeaconId, SignedData>;
signedApiUrls: Record<ChainId, Record<ProviderName, SignedApiUrl[]>>;
derivedSponsorWallets: Record<string /* dAPI name or data feed ID */, Hex /* Private key */>;
signedDatas: Record<Hex /* Beacon ID */, SignedData>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Keccak256Hash for beacon Ids?

Copy link
Collaborator Author

@Siegrift Siegrift Apr 24, 2024

Choose a reason for hiding this comment

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

TBH I am not a great fan of that type. It's quite verbose to read/type and I don't see the benefit compared to Hex. I'd just stick with Hex unless there is a better reason for swithcing.

Also, TS will treat the Hex and Keccak256Hash as the same - so in the future if these are mixed it may cause unnecessary confusion.

src/types.ts Outdated Show resolved Hide resolved
src/update-feeds-loops/contracts.ts Show resolved Hide resolved
Base automatically changed from signed-api-stagger to main April 24, 2024 12:53
@Siegrift Siegrift merged commit 43c41ed into main Apr 25, 2024
4 checks passed
@Siegrift Siegrift deleted the use-commons branch April 25, 2024 08: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.

Use types from@api3/commons
2 participants