Closed gianfra-t closed 5 months ago
github-actions[bot]
commented
5 months ago yarn.lock changes| Status | Count |
|---|---|
| 286 | |
| 64 | |
| 1 |
gianfra-t
commented
5 months ago @ebma regarding the linting issues, there are 2 that I decided to deactivate the warning for:
Here it was complaining about some variables not being listed as dependency of the hook. In this particular case I think we can either ignore it or refactor how the processes are started entirely, since the intention of this hook is to only trigger upon change of the state variable.
And here we have defined an async promise handler which seems to be also bad practice. Since that logic I have not tested but we know works from the other repos we have, I would say we first test it and then we can refactor that part.
ebma
commented
5 months ago I fixed those two issues as well. It's risky to ignore the useEffect dependency warning. In our case it could have happened that the hook would just call the respective functions with outdated secrets and we would spend a lot of time debugging mysterious bugs.
I think this solves #17. We should wait for someone to test the whole flow. And we should also not merge this branch for now and keep the changes on the extra branch.
gianfra-t
commented
5 months ago @ebma I agree with the hook issue, but wouldn't the new refactor of the promise return as soon as the transaction is signed, and not wait for the actual event emitted? Previously we where passing the resolve handler to the callback. I am afraid this will return before the block is finalized and not showing the actual event.
ebma
commented
5 months ago Previously we where passing the resolve handler to the callback. I am afraid this will return before the block is finalized and not showing the actual event.
Ohh yes, thanks for double-checking that! I changed it again, please have another look 😅
gianfra-t
commented
5 months ago This commit should solve the problem where the last cleanup transaction received an incorrect sequence error. This is because I was loading the account three times instead of two like in the tested prototype.
I think maybe we could wait one more test to ensure that all the operations get through and then we can close it right?
ebma
commented
5 months ago Yes, let's wait for confirmation in one more test.
TorstenStueber
commented
5 months ago Same comment here, please change this PR to not merge into main but a special branch such as offramp-prototype (as specified in the original ticket). Let's leave main for the actual product.
netlify[bot]
commented
5 months ago Learn more in the Netlify docs →
| Name | Link |
|---|---|
| Latest commit | 5b4a4cc7d4458f09d27d5cec0a979d9d3a20a171 |
TorstenStueber
commented
5 months ago @gianfra-t @ebma can you please add these lines to App.css – this solves the overflow problem of the wallet modal:
#react-portal-modal-container main {
overflow-y: auto;
}
#react-portal-modal-container > div > div {
max-height: 90vh;
}@gianfra-t seems like you didn't reset the branch to the upstream version and now the changes that I moved to 20-phase-3-of-offramp-prototype are back on this branch again. Can you please remove the commits related to phase 3 again?
gianfra-t
commented
5 months ago Oh sorry! Let me do that.
ebma
commented
5 months ago @gianfra-t Since the testing is completed now, I think we can finally merge this PR to the staging branch and focus only on phase 3 again.
gianfra-t
commented
5 months ago Great, I will close this. Do you know if the changes are deployed automatically on Netlify or we need to trigger manually?
ebma
commented
5 months ago It was automatically deployed to https://offramp-prototype-staging--pendulum-pay.netlify.app/
Possible improvements
Although this is a prototype, there are some checks and improvements that could be improved on a more refined system.
Closes #19