-
Notifications
You must be signed in to change notification settings - Fork 290
Add starknetjs compatibilty tests to CI #1195
Add starknetjs compatibilty tests to CI #1195
Conversation
dc95379
to
f1f062d
Compare
Let's not add the js in the Madara main repo, we can create an action in a separate repo and call it here in the workflow |
Sure thing, let me know to which repo I should add this |
ce077ee
to
bf52d8e
Compare
Ok, I moved the tests to a different repo. One of the tests is failing, I don't really understand why that is the case. The fee is severely underestimated for contracts that are declared in genesis. This error does not pop up for contracts I declare during runtime and I check the fee estimation the same way across the different tests. How do we proceed here? |
cc @tdelabro |
yes we will when it's ready |
@petscheit I would happily merge it in its current state, but the test is failing |
Was being super dumb on Friday, I just pushed the action, so lets include this one also. Regarding the failing test, yup. You need to tell me how to proceed, as I'm measuring the results, and the fee estimation seems to be wrong for genesis contracts. |
@d-roak can you review and approve ? |
@petscheit https://github.com/keep-starknet-strange/madara/actions/runs/6611182918 |
Hmm so the issue is that my action is a workflow correct? And doesn't seem to work. This is what I was struggling with on Friday. Any ideas how to get around this? |
https://github.com/keep-starknet-strange/madara/actions/runs/6611182918
It just look like you missformated you workflow file |
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.
applying both suggestions should fix it
0ff072a
to
106dd1f
Compare
we won't merge it until it's green like a lettuce 🥬 |
nope. @petscheit started investigating it. But he will be busy IRL soon, so he can open and issue with his current advancement and someone will pick it up from there |
106dd1f
to
eb0d0f2
Compare
OK issue fixed, nothing wrong with madara. starknetjs didnt document that you have to pass the constructor calldata for the deploy estimation ( I thought it would be derived via ABI). When I add it, everything works correctly. I also opened a PR on starknetjs to update the docs. |
@d-roak do you approve? |
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.
On this side it's good. The changelog line should be added either in the beginning or the end, but since it's not standardized, the end result will be the same
I would feel more comfortable using the keep-starknet-strange/madara-js-tests
before merging, but if u don't think it's necessary, u can merge it @tdelabro
yeah we can also transfer the repo now, and update the repo link in this PR. |
@petscheit ok lets do this. Ping me on tg when you did the transfer request |
This introduces some tests to ensure compatibility with the starknet-js package. This PR completes #1148
The corresponding testing code has been moved to this repo
Pull Request type
Please add the labels corresponding to the type of changes your PR introduces:
What is the new behavior?
This PR introduces the following tests