Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: add unregister nano contract saga effect [10] #457

Merged
merged 4 commits into from
May 29, 2024
Merged

feat: add unregister nano contract saga effect [10] #457

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
083f44e
feat(nc): implement unregister nano contract saga effect
alexruzenhack Apr 1, 2024
b8edb4e
fix: onNanoContractAddressChangeRequest
alexruzenhack May 29, 2024
36172d9
review: apply suggestions
alexruzenhack May 29, 2024
3ccbcf9
chore: change debug to error
alexruzenhack May 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions __tests__/sagas/nanoContracts/fixtures.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ export const fixtures = {
ncState: {
success: true,
nc_id: '3cb032600bdf7db784800e4ea911b10676fa2f67591f82bb62628c234e771595',
blueprint_id: '000001342d3c5b858a4d4835baea93fcc683fa615ff5892bd044459621a0340a',
blueprint_name: 'Bet',
fields: {
token_uid: { value: '00' },
Expand Down
8 changes: 3 additions & 5 deletions __tests__/sagas/nanoContracts/historyNanoContract.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,7 @@ describe('sagas/nanoContract/fetchHistory', () => {
// arrange wallet mock
const mockedWallet = {
getNetworkObject: jest.fn(),
storage: {
isAddressMine: jest.fn(),
},
isAddressMine: jest.fn(),
};
// arrange ncApi mock
const mockedNcApi = jest.mocked(ncApi);
Expand Down Expand Up @@ -115,10 +113,10 @@ describe('sagas/nanoContract/requestHistoryNanoContract', () => {

// call effect to request history
const gen = requestHistoryNanoContract(nanoContractHistoryRequest({ ncId }));
// select wallet
// select historyMeta
gen.next();
// feed back historyMeta
gen.next({});
gen.next({ [ncId]: { isLoading: false, after: null } });
// feed back wallet
gen.next(fixtures.wallet.readyAndMine);
// feed back isNanoContractRegistered
Expand Down
1 change: 1 addition & 0 deletions __tests__/sagas/nanoContracts/registerNanoContract.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ describe('sagas/nanoContract/registerNanoContract', () => {
expect(actionResult.payload.action.payload.entryValue).toEqual(expect.objectContaining({
address,
ncId,
blueprintId: expect.any(String),
blueprintName: expect.any(String),
}));
// assert termination
Expand Down
40 changes: 40 additions & 0 deletions src/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,12 @@ export const types = {
NANOCONTRACT_HISTORY_SUCCESS: 'NANOCONTRACT_HISTORY_SUCCESS',
/* It indicates a Nano Contract history failed to load. */
NANOCONTRACT_HISTORY_FAILURE: 'NANOCONTRACT_HISTORY_FAILURE',
/* It initiates an unregistration process of a Nano Contract. */
NANOCONTRACT_UNREGISTER_REQUEST: 'NANOCONTRACT_UNREGISTER_REQUEST',
/* It signals a successful completion of unregistration process. */
NANOCONTRACT_UNREGISTER_SUCCESS: 'NANOCONTRACT_UNREGISTER_SUCCESS',
/* It initiates a process to change the address on registered Nano Contract. */
NANOCONTRACT_ADDRESS_CHANGE_REQUEST: 'NANOCONTRACT_ADDRESS_CHANGE_REQUEST',
};

export const featureToggleInitialized = () => ({
Expand Down Expand Up @@ -1055,3 +1061,37 @@ export const nanoContractHistoryFailure = (payload) => ({
type: types.NANOCONTRACT_HISTORY_FAILURE,
payload,
});

/**
* Request unregistration of a Nano Contract by its key.
* @param {{
* ncId: string,
* }} Nano Contract ID to unregister.
*/
export const nanoContractUnregisterRequest = (unregisterRequest) => ({
type: types.NANOCONTRACT_UNREGISTER_REQUEST,
payload: unregisterRequest,
});

/**
* Unregistration of a Nano Contract has finished with success.
* @param {{
* ncId: string,
* }} Nano Contract ID unregistered.
*/
export const nanoContractUnregisterSuccess = (unregistered) => ({
type: types.NANOCONTRACT_UNREGISTER_SUCCESS,
payload: unregistered,
});

/**
* Request a change on the Nano Contract registered.
* @param {{
* ncId: string;
* newAddress: string;
* }} changeAddressRequest
*/
export const nanoContractAddressChangeRequest = (changeAddressRequest) => ({
pedroferreira1 marked this conversation as resolved.
Show resolved Hide resolved
type: types.NANOCONTRACT_ADDRESS_CHANGE_REQUEST,
payload: changeAddressRequest,
});
61 changes: 61 additions & 0 deletions src/reducers/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,10 @@ export const reducer = (state = initialState, action) => {
return onNanoContractHistoryFailure(state, action);
case types.NANOCONTRACT_HISTORY_SUCCESS:
return onNanoContractHistorySuccess(state, action);
case types.NANOCONTRACT_UNREGISTER_SUCCESS:
return onNanoContractUnregisterSuccess(state, action);
case types.NANOCONTRACT_ADDRESS_CHANGE_REQUEST:
return onNanoContractAddressChangeRequest(state, action);
default:
return state;
}
Expand Down Expand Up @@ -1329,6 +1333,37 @@ export const onNanoContractRegisterSuccess = (state, { payload }) => ({
},
});

/**
* @param {Object} state
* @param {{
* payload: {
* ncId: string,
* }
* }} action
*/
export const onNanoContractUnregisterSuccess = (state, { payload }) => {
const { ncId } = payload;

const newRegisteredContracts = { ...state.nanoContract.registered };
delete newRegisteredContracts[ncId];

const newContractsHistory = { ...state.nanoContract.history };
delete newContractsHistory[ncId];

const newContractsHistoryMeta = { ...state.nanoContract.historyMeta };
delete newContractsHistoryMeta[ncId];

return ({
...state,
nanoContract: {
...state.nanoContract,
registered: newRegisteredContracts,
history: newContractsHistory,
historyMeta: newContractsHistoryMeta,
},
});
};

/**
* @param {Object} state
* @param {{
Expand Down Expand Up @@ -1407,3 +1442,29 @@ export const onNanoContractHistorySuccess = (state, { payload }) => ({
},
},
});

/**
* @param {Object} state
* @param {{
* payload: {
* ncId: string;
* newAddress: string;
* }
* }} action
*/
export const onNanoContractAddressChangeRequest = (state, { payload }) => {
const newRegisteredNc = {
...state.nanoContract.registered[payload.ncId],
address: payload.newAddress,
};
return {
...state,
nanoContract: {
...state.nanoContract,
registered: {
...state.nanoContract.registered,
[payload.ncId]: newRegisteredNc,
},
},
};
};
32 changes: 30 additions & 2 deletions src/sagas/nanoContract.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
nanoContractHistorySuccess,
nanoContractRegisterFailure,
nanoContractRegisterSuccess,
nanoContractUnregisterSuccess,
onExceptionCaptured,
types,
} from '../actions';
Expand Down Expand Up @@ -80,7 +81,8 @@ export function* registerNanoContract({ payload }) {

let ncState = null;
try {
ncState = yield call(ncApi.getNanoContractState.bind(ncApi), ncId);
const response = yield call([ncApi, ncApi.getNanoContractState], ncId);
ncState = response.ncState;
} catch (error) {
if (error instanceof NanoRequest404Error) {
yield put(nanoContractRegisterFailure(failureMessage.nanoContractStateNotFound));
Expand All @@ -95,7 +97,7 @@ export function* registerNanoContract({ payload }) {
address,
ncId,
blueprintId: ncState.blueprint_id,
blueprintName: ncState.blueprint_name
blueprintName: ncState.blueprint_name,
};
yield call(wallet.storage.registerNanoContract.bind(wallet.storage), ncId, nc);

Expand Down Expand Up @@ -254,9 +256,35 @@ export function* requestHistoryNanoContract({ payload }) {
}
}

/**
* Process Nano Contract unregister request.
* @param {{
* payload: {
* ncId: string;
* }
* }} action with request payload.
*/
export function* unregisterNanoContract({ payload }) {
const { ncId } = payload;

const wallet = yield select((state) => state.wallet);
if (!wallet.isReady()) {
log.error('Fail unregistering Nano Contract because wallet is not ready yet.');
// This will show user an error modal with the option to send the error to sentry.
yield put(onExceptionCaptured(new Error(failureMessage.walletNotReadyError), true));
andreabadesso marked this conversation as resolved.
Show resolved Hide resolved
return;
}

yield call(wallet.storage.unregisterNanoContract.bind(wallet.storage), ncId);

log.debug(`Success unregistering Nano Contract. ncId = ${ncId}`);
yield put(nanoContractUnregisterSuccess({ ncId }));
}

export function* saga() {
yield all([
takeEvery(types.NANOCONTRACT_REGISTER_REQUEST, registerNanoContract),
takeEvery(types.NANOCONTRACT_HISTORY_REQUEST, requestHistoryNanoContract),
takeEvery(types.NANOCONTRACT_UNREGISTER_REQUEST, unregisterNanoContract),
]);
}
17 changes: 16 additions & 1 deletion src/store.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,21 @@ class HybridStore extends MemoryStore {
async registerNanoContract(ncId, ncValue) {
const contracts = STORE.getItem(REGISTERED_NANO_CONTRACTS_KEY) || {};
contracts[ncId] = ncValue;
STORE.setItem(REGISTERED_NANO_CONTRACTS_KEY, contracts)
STORE.setItem(REGISTERED_NANO_CONTRACTS_KEY, contracts);
}

/**
* Unregister a nano contract.
*
* @param {string} ncId Nano Contract ID.
* @returns {Promise<void>}
* @async
*/
async unregisterNanoContract(ncId) {
await super.unregisterNanoContract(ncId);
const contracts = STORE.getItem(REGISTERED_NANO_CONTRACTS_KEY) || {};
delete contracts[ncId];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you need to set the new object to STORE?

Copy link
Contributor Author

@alexruzenhack alexruzenhack May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't. There aren't a new object to set, contracts is a reference to the object, and we only need remove an entry from this object to set it right. Unless we want to work with immutable objects, which is not the case here. The unregisterToken method sets the registerTokens object after delete, but this operation is redundant.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that the STORE allows us to mutate its stored object directly, but I don't like the idea of potentially multiple places mutating the object directly, I think we should indeed use immutable objects

My suggestion is that we avoid mutating the object in this PR, use STORE.setItem to store it even though it's redundant and open a KTLO to return immutable objects in the Storage

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree we should do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My suggestion is that we avoid mutating the object in this PR, use STORE.setItem to store it even though it's redundant and open a KTLO to return immutable objects in the Storage

Ok. I will open a PR. However, to achieve immutability we need more than just use the spread operator.

Copy link
Contributor

@andreabadesso andreabadesso May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand, we can discuss this in the KTLO, but at the very least we should return a new instance of the object, to prevent the stored object from being mutated by the api consumer

STORE.setItem(REGISTERED_NANO_CONTRACTS_KEY, contracts);
}

/**
Expand All @@ -157,6 +171,7 @@ class HybridStore extends MemoryStore {
if (cleanTokens) {
// Remove from the cache
STORE.removeItem(REGISTERED_TOKENS_KEY);
STORE.removeItem(REGISTERED_NANO_CONTRACTS_KEY);
}
}
}
Expand Down
Loading