Add back chainbridge functionality #29
Conversation
|
Some test seems to be failing, otherwise good work! I’d be interested to have you write up the differences in proposal flows that now exist and how you might imagine relayers integrating and handling multiple proposal flows. Are there issues or ways in which processing can get messy? How can observers discern between different proposal types and what data do they need to differentiate proposals? Do you foresee new vulnerabilities being introduced here, such as if we want to gate this functionality to specific token types? |
|
The failing test is in CompToken.js which I did not tinker with so I assumed this was an error across branches that we were working on. I can attempt to debug if it is just my branch. The error is 'Comp::delegateBySig: signature expired' at line 84 of CompToken.js. In general, I would be happy document and outline flows and potential issues with the multiple proposal types in the system. I will begin work on this before tomorrows meeting. |
|
Master looks to be passing all tests. Our process should converge towards only merging PRs for working and tested code (passing all tests). If there are things broken, we should try our best to fix them.. even if we didn’t create them. |
|
Totally fair. I will fix this for future PR's. Investigating failure now |
Its very strange. Without changing anything I can get the test to pass when running it.only or describe.only for the section of tests its in, but when tested with all of the test files it continues to fail. Because the error has to do with the signature expiring require (now <= expiration) something must change when testing everything. Heading to bed now will see if any insight comes before the meeting |
|
Also before this is merged, we’ll need tests ensuring both flows work interchangeably, I.e that I can move privately and publicly and mint tokens and so on and so forth. |
|
I can make an integration test of sorts to test this.
…On Mon, Aug 30, 2021 at 7:31 PM drewstone ***@***.***> wrote:
Also before this is merged, we’ll need tests ensuring both flows work
interchangeably, I.e that I can move privately and publicly and mint tokens
and so on and so forth.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#29 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMMUR6X7F3UCK6EAVHRRZUTT7QIGTANCNFSM5DA6QDRQ>
.
|
There was a problem hiding this comment.
seems to be some confusion when re-integrating this work. i would suggest not brute force integrating before understanding the overall intentions.
Our goal is to support both functionalities end to end and have tests for those cases primarily. Since the handlers behave differently that includes thinking through proper updates to ensure they both work and behave as expected.
It's good also to integrate all the old tests but the most important is actually none of the ones integrated, e.g. the test of going back and forth using the anchor handler and the deposit handler. This is because there will likely be edge cases around token minting / burning when using our anchor contracts.
Let me know if there's anything confusing about this but at this point this is still not ready.
No description provided.