-
Notifications
You must be signed in to change notification settings - Fork 19
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
feat: P-861 add params support for evm dynamic assertion #2812
Conversation
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.
Thanks! Do you already have an example how it's used in practice?
cc @jonalvarezz - it's a breaking change too
@@ -135,6 +139,6 @@ contract A6 is DynamicAssertion { | |||
pure | |||
returns (string memory) | |||
{ | |||
return concatenateStrings("Bearer ", apiKey); | |||
return string(abi.encodePacked("Bearer ", apiKey)); |
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.
Seems unnecessary unless you want ditch concatenateStrings
everywhere
4ce6916
to
6a5c3be
Compare
let truncated_params = DynamicParams::truncate_from(params); | ||
let truncated_params_len = truncated_params.len(); | ||
if params_len > truncated_params_len { | ||
println!( |
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.
shouldn't this be an error? parameter has already got truncated?
We will use the extra token name parameter in this open PR: #2811 However below I did test the regression flow for the existing solidity VC just to be sure it doesn't break existing ones (which doesn't ask for the extra paramaters yet) either: scripts:
result:
|
Context
Extra arbitrary number parameters are supported now when callling DynamicAssertion requestVc.
This will be immediately handy when we can deploy one holding amount VC contract containing 40+ token in one go, instead of deploy 40+ times each for each one token VC contract.
And in long run solidity VC will support arbitrary VC parameters which will be very useful too.
It's breaking change to interface but given DA is not live in prod the impact is minimized..
Labels
Please apply following PR-related labels when appropriate:
C0-breaking
: if your change could break the existing client, e.g. API change, critical logic changeC1-noteworthy
: if your change is non-breaking, but is still worth noticing for the client, e.g. reference code improvementHow (Optional)
Testing Evidences
Please attach any relevant evidences if applicable