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

Fix AppData computation #35

Merged
merged 3 commits into from
Jan 8, 2025
Merged

Fix AppData computation #35

merged 3 commits into from
Jan 8, 2025

Conversation

fleupold
Copy link
Contributor

@fleupold fleupold commented Jan 8, 2025

After #33, the existing code would generate a newer version of the app data

{"appCode":"CoWFeeModule","environment":"prod","metadata":{},"version":"1.3.0"}
vs
{"appCode":"CoWFeeModule","environment":"prod","metadata":{},"version":"1.1.0"}

which consequently led to this error being hit. @alfetopito suggested to manually generate the app data, given that it's hardcoded and fixed in the fee withdrawl module

Other changes

I had to change the target of the tsconfig to es2022 because es2023 is not supported by my node version (lts, v22.13.0)

@fleupold fleupold requested a review from fedgiac January 8, 2025 10:14
Copy link
Contributor

@fedgiac fedgiac left a comment

Choose a reason for hiding this comment

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

@alfetopito suggested to manually generate the app data, given that it's hardcoded and fixed in the fee withdrawl module

I'd also say we should do this, it'd be less confusing than using a frozen version of the current app data and it would be overall simpler.

tsconfig.json Outdated Show resolved Hide resolved
@fleupold
Copy link
Contributor Author

fleupold commented Jan 8, 2025

I'd also say we should do this, it'd be less confusing than using a frozen version of the current app data and it would be overall simpler.

Just to confirm, this is what this PR does, or do you have something different in mind? I'd still like to send the app data content in plain text to the backend (so we don't have to manually register the hash if we deploy on a new chain).

Copy link
Contributor

@fedgiac fedgiac left a comment

Choose a reason for hiding this comment

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

The changes look good and can be merged.

do you have something different in mind

I was thinking of having appDataHex and appDataContent as global constants instead of using metadataApi.appDataToCid. These constants could even be used in the deployment script: the service in this repo requires some specific app data anyway, so we don't want to allow changing it.

so we don't have to manually register the hash if we deploy on a new chain

This is something we maybe should do as part of the deployment script. We somewhat need to do it anyway for simplicity of reviewing the transaction enabling the module (see example comment here).

@fleupold
Copy link
Contributor Author

fleupold commented Jan 8, 2025

These constants could even be used in the deployment script

This makes sense (one less thing to configure when deploying). I will leave this untackled here though as it seems somehwat low priority.

@fleupold fleupold merged commit 2889bf6 into main Jan 8, 2025
1 check passed
@fleupold fleupold deleted the fix_app_data branch January 8, 2025 16:46
@fleupold fleupold mentioned this pull request Jan 8, 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.

3 participants