Integrate calldata package, drop joi package #1313
Conversation
davidyuk
force-pushed
the
feature/calldata
branch
from
ba99b85 to
3e5a93b
Compare
Codecov Report
@@ Coverage Diff @@
## develop #1313 +/- ##
============================================
+ Coverage 68.34% 83.93% +15.59%
============================================
Files 56 55 -1
Lines 2527 2415 -112
Branches 36 36
============================================
+ Hits 1727 2027 +300
+ Misses 792 372 -420
- Partials 8 16 +8
Continue to review full report at Codecov.
|
|
this PR is not ready to be merged, it needs to be rebased at the top of #1310 |
|
trying to encode
same for trying
|
|
@thepiwo We have two open issues in the calldata lib regarding variants:
Not sure how this is related. But just to let you know. |
| const result = await deployed.callStatic('getArg', ['42']) | ||
| return result.decode().should.eventually.become(42) | ||
| const result = await deployed.callStatic('getArg', [42]) | ||
| expect(result.decodedResult).to.be.equal(42n) |
There was a problem hiding this comment.
why is this with 'n' here?
There was a problem hiding this comment.
it's BigInt which is used in the calldata lib right now. this is also sth. that should be addressed with the integration. I have some open issues in that regards, see:
There was a problem hiding this comment.
we should figure this out or provide a different solution with chai if this is the issue
There was a problem hiding this comment.
@davidyuk can you verify the problem and comment this again?
There was a problem hiding this comment.
I've created a new project with the latest mocha (9.1.3) and chai (4.3.4):
import { expect } from 'chai'
it('works', () => {
expect({ a: 1n }).to.be.eql({ a: 2n })
})mocha test.js outputs:
1) works:
AssertionError: expected { a: 1n } to deeply equal { a: 2n }
+ expected - actual
{
- "a": "1"
+ "a": "2"
}
in the diff section at the end BigInts presented as strings for some reason. If this is the issue, I don't think that we should do anything on our side to avoid that.
There was a problem hiding this comment.
expect({ a: '1' }).to.be.eql({ a: 2n }) {
- "a": "1"
+ "a": "2"
}
🤷♀️
There was a problem hiding this comment.
I still don't get why we have to have it like this. If it stays like this there needs to be a big explainer everywhere for ALL aepp devs to adjust their aepps
There was a problem hiding this comment.
you mean the BigInt in general? well I think in the past we just weren't able to use it and in the calldata lib it makes definitely sense to use it as it is possible now.
I can understand your point of view. but isn't BigInt the right way to do it?
@thepiwo wouldn't you have to touch all repos again anyway with the upcoming AEproject release or how will that transition look like again? (can't exactly remember)
I mean it's definitely a bit unfortunate but it will be a major release - so we could (and maybe should) do it this way 🤷♀️
There was a problem hiding this comment.
No, repos that don't use aeproject don't have to be touched, e.g. governance-server, governance-aepp, superhero wallet, graffiti-aepp, graffiti-server.
We should just make the use of BigInt very clear in docs and Changelogs and so on, I'm not saying its bad to have it.
There was a problem hiding this comment.
we will do that for sure! - this release in general needs a dedicated article / announcement
davidyuk
force-pushed
the
feature/calldata
branch
2 times, most recently
from
30957be to
6a07772
Compare
davidyuk
force-pushed
the
feature/calldata
branch
6 times, most recently
from
e1b18e9 to
5b20d1c
Compare
|
@mradkov I think this one is ready to merge |
davidyuk
force-pushed
the
feature/calldata
branch
from
5b20d1c to
e52c6d8
Compare
closes #1312, closes #435, closes #1253