-
Notifications
You must be signed in to change notification settings - Fork 417
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: warp route deployment verification #3490
Conversation
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3490 +/- ##
=====================================
Coverage 0.00% 0.00%
=====================================
Files 1 1
Lines 16 16
=====================================
Misses 16 16
|
Signed-off-by: Paul Balaji <[email protected]>
Signed-off-by: Paul Balaji <[email protected]>
Signed-off-by: Paul Balaji <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good
please add a changeset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
honestly I would be happy to change the serialization format from
[TokenType]: address
to [ContractName]: address
, which is more consistent with the rest of the codebase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(albeit this is a breaking change)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to use a more multi-protocol format if we're going to change it. I guess [ContractName] is a step in that direction. I'd prefer [TokenStandard] : address
since TokenStandard is essentially the multi-protocol successor to TokenType.
router: contract[contractName as keyof TokenFactories]['address'], | ||
tokenType, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
honestly im not a fan of this different artifact shape
its another unnecessary layer of indirection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[ContractSourceName]: address is more desirable
is this format recovered too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially used this two-file output shape for better consistency with what the other deployers (core, infra) were doing but maybe now's the time to just cut this one and use only the warp-config shape
created an alternative fix that doesn't change any of the warp artifact logic, simply creates a |
closing in favour of #3494 |
should fix the problem whilst also not breaking the format of existing artifact read/writes
drive-by fix to contractverifier - allows propagation of apikeys from custom chain metadata (e.g. chains.yaml)